Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds the DaVinci React Native package, native Android and iOS bridge implementations, sample-app DaVinci wiring, and PingTestRunner scenarios plus unit, integration, and E2E coverage. ChangesDaVinci React Native SDK
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tsdamas
requested review from
pingidentity-gaurav,
rodrigoareis and
witrisna
and removed request for
witrisna
Contributor
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (8)
packages/davinci/src/__tests__/useDavinciForm.test.tsx (1)
181-204: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the updater path really merges.
This test only checks
username. IfsetValues(updater)replaced state instead of merging, it would still pass. Please also assert that an untouched key survives the updater.🤖 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/__tests__/useDavinciForm.test.tsx` around lines 181 - 204, The updater-function path in setValues is only verifying the changed username field, so it could still pass even if the previous state is replaced instead of merged. Update the useDavinciForm.test.tsx case around FormHarness/requireLatest(latest).setValues to also assert that an untouched value from the existing DaVinciFormResult.values remains present after the updater runs, confirming the merging behavior.packages/davinci/src/__tests__/createDaVinciClient.test.ts (1)
382-417: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a concurrent lazy-configure test.
This only proves sequential reuse. A missing in-flight memo would still pass here while overlapping
start()/user()calls double-configure native state. Please add onePromise.all(...)case that assertsconfigureDaVinciruns once across concurrent callers.🤖 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/__tests__/createDaVinciClient.test.ts` around lines 382 - 417, The lazy configuration coverage in createDaVinciClient only verifies sequential reuse and misses the in-flight memo path; add a Promise.all-based test in the createDaVinciClient lazy configure suite that triggers concurrent start() and/or user() calls on the same client and asserts configureDaVinci is called exactly once. Use the existing createDaVinciClient, loadModule, and createNativeMock helpers so the test proves overlapping callers share the same pending configuration instead of double-configuring native state.packages/davinci/package.json (1)
22-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLint script path assumes monorepo structure without guarding.
The
"lint"script hard-codes../../eslint.config.mjs, which will break if this package is ever checked out or used outside the monorepo root. Consider resolving the config path dynamically or documenting the monorepo requirement.🤖 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/package.json` around lines 22 - 30, The package.json lint script in the davinci package hard-codes a monorepo-relative ESLint config path, so update the "lint" command to avoid assuming the repo layout. Adjust the script to resolve the config path dynamically or otherwise make it resilient when run from the package itself, and keep the change localized to the "lint" entry in package.json so it remains usable outside the monorepo root.packages/davinci/RNPingDavinci.podspec (1)
36-36: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse
s.swift_versionshere
swift_versionis the deprecated singular form;swift_versionsis the current podspec DSL for listing supported Swift versions.Suggested fix
- s.swift_version = ['5.0', '5.1', '6.0'] + s.swift_versions = ['5.0', '5.1', '6.0']🤖 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/RNPingDavinci.podspec` at line 36, The podspec is using the deprecated singular Swift version setting, so update the version declaration in RNPingDavinci.podspec from s.swift_version to the supported plural form s.swift_versions. Keep the listed supported versions the same, but switch the property name so the podspec uses the current DSL and avoids deprecation warnings.packages/davinci/android/src/test/java/com/pingidentity/rndavinci/mapper/DaVinciNodeMapperTest.kt (1)
160-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining Android collector serializers.
This suite still skips Android mapper cases for collector types that are part of the public bridge contract in this PR, notably phone-number and device registration/authentication variants. A regression in those serializer branches would ship Android-only schema drift without tripping CI.
🤖 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 160 - 265, Add test coverage in DaVinciNodeMapperTest for the remaining collector serializer branches in DaVinciNodeMapper.mapNodePayload, especially the phone-number and device registration/authentication collector variants mentioned in the review. Create focused tests similar to the existing collector cases to assert the expected base fields and any type-specific payload keys so these public bridge contract paths are exercised by CI.PingTestRunner/e2e/davinci.test.ts (1)
84-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer visible skips over silent early-return for env-gated cases.
When
hasDaVinciEnv()is false the liveit()blocksconsole.warnandreturn, so they report as passed rather than skipped. That can mask real failures in CI where the env is expected to be present, and makes it hard to distinguish "intentionally skipped" from "actually green". Consider gating with a conditionaldescribe/itso the runner marks them as skipped.♻️ Example using a conditional test runner
-describe('DaVinci — happy path', () => { +const liveIt = hasDaVinciEnv() ? it : it.skip; + +describe('DaVinci — happy path', () => {Then use
liveIt(...)for the live cases and drop the per-testif (!hasDaVinciEnv()) { ... return; }guards. NotehasDaVinciEnv()would need to read env at module load, which is fine for launch-arg-driven runs.🤖 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/e2e/davinci.test.ts` around lines 84 - 180, The live DaVinci e2e tests are using per-test `if (!hasDaVinciEnv()) { console.warn(...); return; }`, which makes env-gated cases look passed instead of skipped. Update `PingTestRunner/e2e/davinci.test.ts` to gate these `it()` blocks through a conditional `describe` or a helper like `liveIt(...)` so the runner marks them as skipped, and remove the early-return guards from the individual tests such as `start()`, `next()`, `userinfo()`, `refresh()`, `revoke()`, and `logoutUser()`.PingSampleApp/ui/davinci/components/atoms/DaVinciErrorList.tsx (1)
49-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnounce validation errors to assistive tech.
These errors are only rendered visually right now. When validation fails after submit/change, VoiceOver/TalkBack may not announce the new content, so users can miss why the form is blocked. Please add an alert/live-region semantic on the wrapper.
Suggested change
- <View> + <View + accessible + accessibilityRole="alert" + accessibilityLiveRegion="polite" + > {errors.map((error, index) => ( <Text key={`${error.code}-${index}`} style={davinciFieldStyles.errorText} >🤖 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/atoms/DaVinciErrorList.tsx` around lines 49 - 59, Add an assistive-technology announcement role to the error wrapper in DaVinciErrorList so validation updates are spoken, not just rendered visually. Update the View returned by DaVinciErrorList to use an alert/live-region semantic (for example via the wrapper’s accessibility props) while keeping the existing error mapping and describeError rendering intact. Ensure the change is localized to the DaVinciErrorList component and does not alter the error text content or keys.PingSampleApp/ui/davinci/components/atoms/DaVinciFieldLabel.tsx (1)
36-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDon't rely on
*alone for required fields.The asterisk is only a visual cue. Screen readers may read it literally instead of conveying that the field is required, so the accessible contract is weaker than the visual one. Please expose the required state in accessible text as well.
Suggested change
- <Text style={davinciFieldStyles.fieldLabel}> + <Text + style={davinciFieldStyles.fieldLabel} + accessibilityLabel={required ? `${label} required` : label} + > {label} {required ? <Text style={davinciFieldStyles.required}> *</Text> : null} </Text>🤖 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/atoms/DaVinciFieldLabel.tsx` around lines 36 - 40, The required-state UI in DaVinciFieldLabel currently relies on a visual asterisk only, so update the DaVinciFieldLabel component to expose the required status in accessible text as well. Keep the existing label rendering, but add an accessibility-friendly indication for required fields in the same component (for example via text that screen readers can announce) alongside the visual marker. Locate the change in DaVinciFieldLabel and ensure the required branch remains consistent for both sighted and assistive users.
🤖 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/collector/DaVinciCollectorValueApplier.kt`:
- Around line 161-168: The asString helper in DaVinciCollectorValueApplier is
converting all numeric inputs to strings via Number.toString(), which causes
whole-number JS values coming from readDynamicValue to be applied as malformed
strings like "5.0". Update asString to detect numeric values that represent
whole numbers and stringify them without the trailing decimal, while preserving
non-integer decimals and the existing handling for String and Boolean. Keep the
fix localized to asString so the collector receives the expected text for
single-select/text fields.
In
`@packages/davinci/android/src/test/java/com/pingidentity/rndavinci/RNPingDavinciCommonTest.kt`:
- Around line 497-499: The await helper in RNPingDavinciCommonTest silently
ignores the boolean result from latch.await, so timed-out async work can
continue and fail later in a confusing way. Update await(timeoutMs) to check the
return value from latch.await and fail immediately when it returns false, using
the existing await/latch helper path in RNPingDavinciCommonTest so hung
Promise-based tests report a clear timeout instead of a later null assertion.
In `@packages/davinci/ios/Collector/DaVinciCollectorValueApplier.swift`:
- Around line 179-194: The asString helper in DaVinciCollectorValueApplier is
treating bridged NSNumber 0/1 values as Bool, which converts numeric collector
values into true/false. Update the type checks in asString(_ value:key:) so the
Bool branch only matches actual booleans, then let NSNumber values continue
through the numeric path to preserve "0"/"1" output.
In `@packages/davinci/ios/Mapper/DaVinciNodeMapper.swift`:
- Around line 108-128: Use collector.key consistently when matching form fields
in unsupportedFields(_:logger:), because FieldCollector.id is not the field key
and can incorrectly classify valid fields as unsupported. Update the
registeredKeys set to collect collector.key values, and ensure the raw field
lookup/path in DaVinciNodeMapper aligns with the same key-based comparison so
supported fields are preserved.
In `@packages/davinci/ios/Tests/DaVinciClientFactoryTests.swift`:
- Around line 18-24: `tearDown()` in `DaVinciClientFactoryTests` is starting
cleanup in a detached `Task` and returning before
`CoreRuntime.oidcStorageConfigRegistry` and `CoreRuntime.loggerRegistry` are
actually cleared. Make the cleanup synchronous from the test’s perspective by
awaiting both `removeAll()` calls before `tearDown()` exits, so the next test
can’t observe stale registry state.
In `@packages/davinci/ios/Tests/DaVinciCollectorValueApplierTests.swift`:
- Around line 193-204: The phone collector test currently accepts a
non-dictionary payload and leaves the existing countryCode/phoneNumber
untouched, but it should verify that DaVinciCollectorValueApplier.apply rejects
this malformed input with a DaVinciBridgeError.argument instead. Update the test
in testApplySkipsPhoneFieldsForNonDictionary to assert the error path for the
CollectorMutation keyed by "phone", aligning it with the other invalid-input
cases in DaVinciCollectorValueApplier and makePhoneNumberCollector.
In `@packages/davinci/ios/Tests/DaVinciConfigParserTests.swift`:
- Around line 164-174: The timeout parsing in DaVinciConfigParser.parse
currently treats an invalid string like "not-a-number" as nil, which silently
falls back to no timeout; change the parser to validate the timeout field and
throw an argument error for malformed values instead of returning a payload with
a nil timeout. Update the timeout conversion logic in DaVinciConfigParser and
adjust the related tests so the invalid input case expects a failure rather than
XCTAssertNil on payload.timeout.
In `@packages/davinci/ios/Tests/RNPingDavinciCommonTests.swift`:
- Around line 64-70: The teardown in RNPingDavinciCommonTests is firing off
logger cleanup asynchronously instead of waiting for it, which can leak
CoreRuntime.loggerRegistry state into later tests. Update the test teardown path
around tearDown and CoreRuntime.loggerRegistry.removeAll() so the cleanup is
awaited before finishing teardown, using an async teardown pattern rather than a
detached Task. Keep the existing RNPingDavinciCommon.cleanup() call, but ensure
the logger registry removal completes synchronously from the test framework’s
perspective.
In `@packages/davinci/package.json`:
- Around line 39-43: The peerDependencies in the package manifest are too
permissive because react and react-native use wildcard ranges, so tighten them
to the minimum supported compatible semver ranges instead of allowing any
version. Update the peer dependency entries in the package manifest to explicit
caret ranges that match what this package supports, keeping
`@ping-identity/rn-core` as-is and constraining react and react-native to
compatible versions so consumers get install-time warnings for unsupported
majors.
In `@packages/davinci/src/davinci.ts`:
- Around line 191-203: ensureConfigured in davinci.ts is not concurrency-safe
because it only checks davinciId before awaiting configureDaVinci, so concurrent
callers like start() and user() can trigger duplicate native configuration. Fix
this by caching the in-flight promise inside ensureConfigured (or alongside
davinciId) so all overlapping calls await the same
configureDaVinci(nativeConfig) work, then store davinciId once it resolves and
clear the cached promise on success or failure.
In `@PingSampleApp/ui/davinci/components/organisms/DaVinciContinueNodePanel.tsx`:
- Around line 56-64: The interactive-submit check in hasInteractiveSubmit() is
missing FLOW_LINK, so link-only nodes fall through to the fallback Next button
and trigger onSubmit() instead of the flow action path. Update
hasInteractiveSubmit() to treat FLOW_LINK as interactive alongside
SUBMIT_BUTTON, ACTION, and FLOW_BUTTON, and ensure the fallback rendering in
DaVinciContinueNodePanel uses this updated detection so nodes with only
FLOW_LINK do not show the incorrect submit button.
In `@PingSampleApp/ui/davinci/hooks/useDaVinciAutoStartEffect.ts`:
- Around line 45-51: The auto-start latch in useDaVinciAutoStartEffect is only
cleared when onStart() returns false, so a rejected onStart() leaves
hasAutoStartedRef.current stuck true and creates an unhandled rejection. Update
the async block around onStart() to catch failures, reset
hasAutoStartedRef.current back to false on error, and ensure the failure is
handled/logged so the hook can retry on the same screen instance.
In `@PingTestRunner/jest.setup.js`:
- Around line 69-84: The native mock in jest.setup.js is too permissive because
configureDaVinci always returns a fixed id and the other methods never verify
which client instance is being used. Update the mock for NativeRNPingDavinci so
it tracks the configured client identity and validates that start, next,
getSession, refresh, revoke, userinfo, logout, and dispose are called against
the same configured id; also make disposed clients reject further calls. This
will let PingTestRunner catch JS bridge bugs where configureDaVinci is skipped,
the wrong client id is forwarded, or a disposed client is reused.
In `@PingTestRunner/scenarios/DaVinciScenario.tsx`:
- Around line 190-207: The success flags in handleRevoke and handleLogout are
being set even when client.revoke() or client.logoutUser() throws, so move
setRevoked(true) and setLoggedOut(true) to only run after the await succeeds
inside the try block. Keep the existing setRuntimeError handling in the catch
path, and use the handleRevoke/handleLogout callbacks to ensure Detox only sees
davinci-revoked or davinci-logged-out after a successful native call.
- Around line 151-166: `handleFlow()` is overwriting the current form payload by
sending only the flow key, which drops existing collector values; update the
`handleFlow` callback in `DaVinciScenario` to merge the flow action into the
current payload instead of replacing it. Reuse the existing form state used by
`handleSubmit`/`buildPayload` and ensure the submitted collectors include both
the current values and the `{ key: flowKey, value: flowKey }` entry so flow
actions preserve prior inputs.
---
Nitpick comments:
In
`@packages/davinci/android/src/test/java/com/pingidentity/rndavinci/mapper/DaVinciNodeMapperTest.kt`:
- Around line 160-265: Add test coverage in DaVinciNodeMapperTest for the
remaining collector serializer branches in DaVinciNodeMapper.mapNodePayload,
especially the phone-number and device registration/authentication collector
variants mentioned in the review. Create focused tests similar to the existing
collector cases to assert the expected base fields and any type-specific payload
keys so these public bridge contract paths are exercised by CI.
In `@packages/davinci/package.json`:
- Around line 22-30: The package.json lint script in the davinci package
hard-codes a monorepo-relative ESLint config path, so update the "lint" command
to avoid assuming the repo layout. Adjust the script to resolve the config path
dynamically or otherwise make it resilient when run from the package itself, and
keep the change localized to the "lint" entry in package.json so it remains
usable outside the monorepo root.
In `@packages/davinci/RNPingDavinci.podspec`:
- Line 36: The podspec is using the deprecated singular Swift version setting,
so update the version declaration in RNPingDavinci.podspec from s.swift_version
to the supported plural form s.swift_versions. Keep the listed supported
versions the same, but switch the property name so the podspec uses the current
DSL and avoids deprecation warnings.
In `@packages/davinci/src/__tests__/createDaVinciClient.test.ts`:
- Around line 382-417: The lazy configuration coverage in createDaVinciClient
only verifies sequential reuse and misses the in-flight memo path; add a
Promise.all-based test in the createDaVinciClient lazy configure suite that
triggers concurrent start() and/or user() calls on the same client and asserts
configureDaVinci is called exactly once. Use the existing createDaVinciClient,
loadModule, and createNativeMock helpers so the test proves overlapping callers
share the same pending configuration instead of double-configuring native state.
In `@packages/davinci/src/__tests__/useDavinciForm.test.tsx`:
- Around line 181-204: The updater-function path in setValues is only verifying
the changed username field, so it could still pass even if the previous state is
replaced instead of merged. Update the useDavinciForm.test.tsx case around
FormHarness/requireLatest(latest).setValues to also assert that an untouched
value from the existing DaVinciFormResult.values remains present after the
updater runs, confirming the merging behavior.
In `@PingSampleApp/ui/davinci/components/atoms/DaVinciErrorList.tsx`:
- Around line 49-59: Add an assistive-technology announcement role to the error
wrapper in DaVinciErrorList so validation updates are spoken, not just rendered
visually. Update the View returned by DaVinciErrorList to use an
alert/live-region semantic (for example via the wrapper’s accessibility props)
while keeping the existing error mapping and describeError rendering intact.
Ensure the change is localized to the DaVinciErrorList component and does not
alter the error text content or keys.
In `@PingSampleApp/ui/davinci/components/atoms/DaVinciFieldLabel.tsx`:
- Around line 36-40: The required-state UI in DaVinciFieldLabel currently relies
on a visual asterisk only, so update the DaVinciFieldLabel component to expose
the required status in accessible text as well. Keep the existing label
rendering, but add an accessibility-friendly indication for required fields in
the same component (for example via text that screen readers can announce)
alongside the visual marker. Locate the change in DaVinciFieldLabel and ensure
the required branch remains consistent for both sighted and assistive users.
In `@PingTestRunner/e2e/davinci.test.ts`:
- Around line 84-180: The live DaVinci e2e tests are using per-test `if
(!hasDaVinciEnv()) { console.warn(...); return; }`, which makes env-gated cases
look passed instead of skipped. Update `PingTestRunner/e2e/davinci.test.ts` to
gate these `it()` blocks through a conditional `describe` or a helper like
`liveIt(...)` so the runner marks them as skipped, and remove the early-return
guards from the individual tests such as `start()`, `next()`, `userinfo()`,
`refresh()`, `revoke()`, and `logoutUser()`.
🪄 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: e370fdf0-8d47-44ee-977f-995f7a22adfb
⛔ 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 (120)
.gitignorePingSampleApp/App.tsxPingSampleApp/package.jsonPingSampleApp/src/clients.tsPingSampleApp/src/styles/davinciStyles.tsPingSampleApp/ui/DaVinciScreen.tsxPingSampleApp/ui/HomeScreen.tsxPingSampleApp/ui/LogoutScreen.tsxPingSampleApp/ui/TokenScreen.tsxPingSampleApp/ui/UserProfileScreen.tsxPingSampleApp/ui/davinci/README.mdPingSampleApp/ui/davinci/components/atoms/DaVinciErrorList.tsxPingSampleApp/ui/davinci/components/atoms/DaVinciFieldLabel.tsxPingSampleApp/ui/davinci/components/atoms/PickerModal.tsxPingSampleApp/ui/davinci/components/molecules/DaVinciDeviceField.tsxPingSampleApp/ui/davinci/components/molecules/DaVinciFieldRenderer.tsxPingSampleApp/ui/davinci/components/molecules/DaVinciFlowButton.tsxPingSampleApp/ui/davinci/components/molecules/DaVinciLabelField.tsxPingSampleApp/ui/davinci/components/molecules/DaVinciMultiSelectField.tsxPingSampleApp/ui/davinci/components/molecules/DaVinciPasswordField.tsxPingSampleApp/ui/davinci/components/molecules/DaVinciPasswordRequirements.tsxPingSampleApp/ui/davinci/components/molecules/DaVinciPhoneNumberField.tsxPingSampleApp/ui/davinci/components/molecules/DaVinciSingleSelectField.tsxPingSampleApp/ui/davinci/components/molecules/DaVinciSubmitButton.tsxPingSampleApp/ui/davinci/components/molecules/DaVinciTextField.tsxPingSampleApp/ui/davinci/components/molecules/DaVinciUnsupportedField.tsxPingSampleApp/ui/davinci/components/molecules/types.tsPingSampleApp/ui/davinci/components/organisms/DaVinciClientPanel.tsxPingSampleApp/ui/davinci/components/organisms/DaVinciContinueNodePanel.tsxPingSampleApp/ui/davinci/hooks/useDaVinciAutoStartEffect.tsPingSampleApp/ui/davinci/hooks/useDaVinciClientPanelController.tsPingSampleApp/ui/davinci/hooks/useDaVinciSessionController.tsPingSampleApp/ui/token/components/organisms/TokenDaVinciPanel.tsxPingSampleApp/ui/userProfile/components/organisms/UserProfileDaVinciPanel.tsxPingTestRunner/App.tsxPingTestRunner/__tests__/integration/davinci.test.tsPingTestRunner/__tests__/integration/native-spec-contracts.test.tsPingTestRunner/e2e/davinci.test.tsPingTestRunner/ios/PodfilePingTestRunner/jest.config.jsPingTestRunner/jest.setup.jsPingTestRunner/package.jsonPingTestRunner/scenarios/DaVinciScenario.tsxPingTestRunner/scenarios/UseDaVinciScenario.tsxPingTestRunner/scripts/test-native-android.shpackages/core/android/src/main/java/com/pingidentity/rncore/CoreRuntime.ktpackages/core/ios/CoreRuntime.swiftpackages/davinci/.editorconfigpackages/davinci/.gitattributespackages/davinci/.gitignorepackages/davinci/LICENSEpackages/davinci/README.mdpackages/davinci/RNPingDavinci.podspecpackages/davinci/android/build.gradlepackages/davinci/android/src/main/AndroidManifest.xmlpackages/davinci/android/src/main/java/com/pingidentity/rndavinci/RNPingDavinciCommon.ktpackages/davinci/android/src/main/java/com/pingidentity/rndavinci/collector/DaVinciCollectorValueApplier.ktpackages/davinci/android/src/main/java/com/pingidentity/rndavinci/config/DaVinciConfigParser.ktpackages/davinci/android/src/main/java/com/pingidentity/rndavinci/error/DaVinciErrorCodes.ktpackages/davinci/android/src/main/java/com/pingidentity/rndavinci/error/DaVinciErrorMapper.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/newarch/java/com/pingidentity/rndavinci/RNPingDavinciPackage.ktpackages/davinci/android/src/oldarch/java/com/pingidentity/rndavinci/RNPingDavinciClassicModule.ktpackages/davinci/android/src/oldarch/java/com/pingidentity/rndavinci/RNPingDavinciPackage.ktpackages/davinci/android/src/test/java/com/pingidentity/rndavinci/RNPingDavinciCommonTest.ktpackages/davinci/android/src/test/java/com/pingidentity/rndavinci/collector/DaVinciCollectorValueApplierTest.ktpackages/davinci/android/src/test/java/com/pingidentity/rndavinci/config/DaVinciConfigParserTest.ktpackages/davinci/android/src/test/java/com/pingidentity/rndavinci/error/DaVinciErrorMapperTest.ktpackages/davinci/android/src/test/java/com/pingidentity/rndavinci/factory/DaVinciClientFactoryTest.ktpackages/davinci/android/src/test/java/com/pingidentity/rndavinci/mapper/DaVinciNodeMapperTest.ktpackages/davinci/babel.config.jspackages/davinci/eslint.config.mjspackages/davinci/ios/Collector/DaVinciCollectorValueApplier.swiftpackages/davinci/ios/Config/DaVinciConfigParser.swiftpackages/davinci/ios/Error/DaVinciErrorCodes.swiftpackages/davinci/ios/Error/DaVinciErrorMapper.swiftpackages/davinci/ios/Factory/DaVinciClientFactory.swiftpackages/davinci/ios/Mapper/DaVinciNodeMapper.swiftpackages/davinci/ios/Models/DaVinciPayloads.swiftpackages/davinci/ios/RNPingDavinci.hpackages/davinci/ios/RNPingDavinci.mmpackages/davinci/ios/RNPingDavinciClassic.mmpackages/davinci/ios/RNPingDavinciCommon.swiftpackages/davinci/ios/RNPingDavinciImpl.swiftpackages/davinci/ios/Tests/DaVinciClientFactoryTests.swiftpackages/davinci/ios/Tests/DaVinciCollectorValueApplierTests.swiftpackages/davinci/ios/Tests/DaVinciConfigParserTests.swiftpackages/davinci/ios/Tests/DaVinciErrorMapperTests.swiftpackages/davinci/ios/Tests/DaVinciNodeMapperTests.swiftpackages/davinci/ios/Tests/RNPingDavinciCommonTests.swiftpackages/davinci/ios/Tests/RNPingDavinciImplTests.swiftpackages/davinci/jest.config.jspackages/davinci/jest.setup.jspackages/davinci/package.jsonpackages/davinci/src/NativeRNPingDavinci.tspackages/davinci/src/__tests__/collectorHelpers.test.tspackages/davinci/src/__tests__/createDaVinciClient.test.tspackages/davinci/src/__tests__/davinci.test.tspackages/davinci/src/__tests__/davinciMethods.test.tspackages/davinci/src/__tests__/index.test.tsxpackages/davinci/src/__tests__/nativeModuleAndProvider.test.tsxpackages/davinci/src/__tests__/useDavinci.test.tsxpackages/davinci/src/__tests__/useDavinciForm.test.tsxpackages/davinci/src/collectorHelpers.tspackages/davinci/src/davinci.tspackages/davinci/src/davinciMethods.tspackages/davinci/src/index.tsxpackages/davinci/src/types/client.types.tspackages/davinci/src/types/config.types.tspackages/davinci/src/types/error.types.tspackages/davinci/src/types/form.types.tspackages/davinci/src/types/index.tspackages/davinci/src/types/node.types.tspackages/davinci/src/useDavinci.tsxpackages/davinci/src/useDavinciForm.tspackages/davinci/tsconfig.build.jsonpackages/davinci/tsconfig.jsonpackages/davinci/turbo.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/davinci/ios/Tests/RNPingDavinciCommonTests.swift (1)
64-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate the two
tearDownoverrides.The fire-and-forget
Taskflagged previously is now gone, which resolves the async-cleanup concern. However, you now have both a synchronoustearDown()and an asynctearDown() async throws. XCTest will invoke both, soRNPingDavinciCommon.cleanup()andCoreRuntime.loggerRegistry.removeAll()run in separate teardown passes. Folding both into the single async teardown keeps the cleanup sequence in one place and avoids ambiguity about ordering.♻️ Suggested consolidation
- override func tearDown() { - RNPingDavinciCommon.cleanup() - super.tearDown() - } - - override func tearDown() async throws { - await CoreRuntime.loggerRegistry.removeAll() - try await super.tearDown() - } + override func tearDown() async throws { + RNPingDavinciCommon.cleanup() + await CoreRuntime.loggerRegistry.removeAll() + try await super.tearDown() + }🤖 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/RNPingDavinciCommonTests.swift` around lines 64 - 72, The test teardown logic is split across both `RNPingDavinciCommonTests.tearDown()` and `RNPingDavinciCommonTests.tearDown() async throws`, so XCTest will run cleanup in two separate passes. Consolidate the cleanup into the async `tearDown` override by moving `RNPingDavinciCommon.cleanup()` and `CoreRuntime.loggerRegistry.removeAll()` there, then call `super.tearDown()` once from that method and remove the synchronous override.
🤖 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/package.json`:
- Line 22: The BrowserStack Android journey script was renamed to
test:bs:android:journey-davinci, but existing callers still expect
test:bs:android:journey. Fix this by either restoring test:bs:android:journey as
an alias in package.json or updating all usages to the new script name,
especially the BrowserStack journey workflow and any other references to the old
npm script. Keep the script name(s) consistent so the journey pipeline continues
to run.
---
Nitpick comments:
In `@packages/davinci/ios/Tests/RNPingDavinciCommonTests.swift`:
- Around line 64-72: The test teardown logic is split across both
`RNPingDavinciCommonTests.tearDown()` and `RNPingDavinciCommonTests.tearDown()
async throws`, so XCTest will run cleanup in two separate passes. Consolidate
the cleanup into the async `tearDown` override by moving
`RNPingDavinciCommon.cleanup()` and `CoreRuntime.loggerRegistry.removeAll()`
there, then call `super.tearDown()` once from that method and remove the
synchronous override.
🪄 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: 9871a6bf-4b21-4997-b9f8-0cb8ce04571a
📒 Files selected for processing (16)
PingSampleApp/ui/davinci/components/organisms/DaVinciContinueNodePanel.tsxPingSampleApp/ui/davinci/hooks/useDaVinciAutoStartEffect.tsPingTestRunner/__tests__/integration/davinci.test.tsPingTestRunner/jest.setup.jsPingTestRunner/package.jsonPingTestRunner/scenarios/DaVinciScenario.tsxpackages/davinci/android/src/main/java/com/pingidentity/rndavinci/collector/DaVinciCollectorValueApplier.ktpackages/davinci/android/src/test/java/com/pingidentity/rndavinci/RNPingDavinciCommonTest.ktpackages/davinci/android/src/test/java/com/pingidentity/rndavinci/collector/DaVinciCollectorValueApplierTest.ktpackages/davinci/ios/Collector/DaVinciCollectorValueApplier.swiftpackages/davinci/ios/Config/DaVinciConfigParser.swiftpackages/davinci/ios/Tests/DaVinciClientFactoryTests.swiftpackages/davinci/ios/Tests/DaVinciConfigParserTests.swiftpackages/davinci/ios/Tests/RNPingDavinciCommonTests.swiftpackages/davinci/src/davinci.tspackages/davinci/src/useDavinci.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- PingTestRunner/jest.setup.js
- PingSampleApp/ui/davinci/components/organisms/DaVinciContinueNodePanel.tsx
- PingSampleApp/ui/davinci/hooks/useDaVinciAutoStartEffect.ts
- packages/davinci/ios/Tests/DaVinciConfigParserTests.swift
- packages/davinci/ios/Tests/DaVinciClientFactoryTests.swift
- PingTestRunner/tests/integration/davinci.test.ts
- PingTestRunner/scenarios/DaVinciScenario.tsx
- packages/davinci/src/davinci.ts
- packages/davinci/android/src/test/java/com/pingidentity/rndavinci/RNPingDavinciCommonTest.kt
- packages/davinci/ios/Collector/DaVinciCollectorValueApplier.swift
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (48.42%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #57 +/- ##
============================================
- Coverage 51.39% 48.42% -2.97%
- Complexity 82 168 +86
============================================
Files 189 212 +23
Lines 15597 17972 +2375
Branches 651 898 +247
============================================
+ Hits 8016 8703 +687
- Misses 7514 9147 +1633
- Partials 67 122 +55
... and 36 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
PingTestRunner/e2e/setup.ts (1)
71-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a
hasDaVinciEnv()guard for consistency.
E2E_ENVhashasLiveAuthEnv()/hasJourneyEnv()to gate tests when credentials aren't configured;DAVINCI_ENVhas no equivalent, so DaVinci specs may run unconditionally even with empty strings for required PingOne values.♻️ Proposed addition
+export function hasDaVinciEnv(): boolean { + return !!( + DAVINCI_ENV.discoveryEndpoint && + DAVINCI_ENV.clientId && + DAVINCI_ENV.testUsername && + DAVINCI_ENV.testPassword + ); +}🤖 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/e2e/setup.ts` around lines 71 - 80, Add a hasDaVinciEnv() guard alongside DAVINCI_ENV so DaVinci tests are skipped when required PingOne values are missing. Update the setup in setup.ts to check the DAVINCI_ENV fields used by the DaVinci specs (such as discoveryEndpoint, clientId, testUsername, testPassword, and acrValues) and return a boolean like the existing E2E_ENV helpers hasLiveAuthEnv() and hasJourneyEnv(). Then use that helper in the DaVinci test gating so specs do not run with empty configuration.
🤖 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/package.json`:
- Line 23: The test script in PingTestRunner/package.json is using an escaped
regex group in the `test:bs:android:journey-davinci` command, so
`--testPathPattern` matches the literal parentheses instead of the intended
files. Update that script to use the unescaped `e2e/(journey|davinci)` pattern
so Detox selects the `e2e/journey...` and `e2e/davinci...` tests correctly.
---
Nitpick comments:
In `@PingTestRunner/e2e/setup.ts`:
- Around line 71-80: Add a hasDaVinciEnv() guard alongside DAVINCI_ENV so
DaVinci tests are skipped when required PingOne values are missing. Update the
setup in setup.ts to check the DAVINCI_ENV fields used by the DaVinci specs
(such as discoveryEndpoint, clientId, testUsername, testPassword, and acrValues)
and return a boolean like the existing E2E_ENV helpers hasLiveAuthEnv() and
hasJourneyEnv(). Then use that helper in the DaVinci test gating so specs do not
run with empty configuration.
🪄 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: 87dff034-b638-4c8a-8bb8-b543f85eac2a
📒 Files selected for processing (8)
.github/workflows/browserstack-e2e-android-journey.yml.github/workflows/ci.ymlPingTestRunner/.env.examplePingTestRunner/e2e/davinci.test.tsPingTestRunner/e2e/setup.tsPingTestRunner/package.jsonPingTestRunner/scenarios/DaVinciScenario.tsxpackage.json
🚧 Files skipped from review as they are similar to previous changes (2)
- PingTestRunner/e2e/davinci.test.ts
- PingTestRunner/scenarios/DaVinciScenario.tsx
| "test:e2e:android": "sh -c 'export DETOX_SERVER_PORT=${DETOX_SERVER_PORT:-8100}; export NODE_NO_WARNINGS=1; yarn e2e:android:free-detox-port && yarn e2e:android:kill-emulators && detox test --configuration android.emu --cleanup \"$@\"' --", | ||
| "test:bs:android": "NODE_NO_WARNINGS=1 detox test --configuration android.bs --loglevel trace", | ||
| "test:bs:android:journey": "NODE_NO_WARNINGS=1 detox test --configuration android.bs --loglevel trace --testPathPattern journey", | ||
| "test:bs:android:journey-davinci": "NODE_NO_WARNINGS=1 detox test --configuration android.bs --loglevel trace --testPathPattern 'e2e/\\(journey\\|davinci\\)'", |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node -e "
const re = new RegExp('e2e/\\\\(journey\\\\|davinci\\\\)', 'i');
console.log('escaped pattern matches journey.test.ts:', re.test('e2e/journey.test.ts'));
console.log('escaped pattern matches davinci.test.ts:', re.test('e2e/davinci.test.ts'));
const re2 = new RegExp('e2e/(journey|davinci)', 'i');
console.log('unescaped pattern matches journey.test.ts:', re2.test('e2e/journey.test.ts'));
console.log('unescaped pattern matches davinci.test.ts:', re2.test('e2e/davinci.test.ts'));
"Repository: ForgeRock/ping-react-native-sdk
Length of output: 359
testPathPattern needs the unescaped regex group. PingTestRunner/package.json:23
--testPathPattern is treated as a single RegExp, so e2e/\\(journey\\|davinci\\) matches a literal (journey|davinci) string and won’t select e2e/journey... or e2e/davinci.... Use e2e/(journey|davinci) instead.
🤖 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/package.json` at line 23, The test script in
PingTestRunner/package.json is using an escaped regex group in the
`test:bs:android:journey-davinci` command, so `--testPathPattern` matches the
literal parentheses instead of the intended files. Update that script to use the
unescaped `e2e/(journey|davinci)` pattern so Detox selects the `e2e/journey...`
and `e2e/davinci...` tests correctly.
Contributor
Author
There was a problem hiding this comment.
Good catch on the pattern, but the escaping is intentional and consistent with the pre-existing test:bs:android:core script on the line below, which uses the same \( / \| form. In JSON, \( deserialises to (, and when that string is passed through the shell (inside single quotes) to Detox/Jest, the regex engine receives (journey|davinci) — which Jest treats as an alternation group because ( and ) are the escaped-group syntax in BRE/ERE-style patterns, and | is the alternation operator. The core script has been working correctly with this convention, so I've kept journey-davinci consistent with it.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node -e "
const paths = [
'e2e/app-launch.test.ts',
'e2e/oidc.test.ts',
'e2e/journey.test.ts',
'e2e/davinci.test.ts'
];
const escapedCore = new RegExp('e2e/\\\\(app-launch\\\\|app-open-close\\\\|browser\\\\|device-id\\\\|device-profile\\\\|logger\\\\|oidc\\\\|storage\\\\|use-oidc\\\\)', 'i');
console.log('--- escaped core pattern (as currently written in package.json) ---');
console.log('source:', escapedCore.source);
for (const p of paths) console.log(p, '=>', escapedCore.test(p));
const unescapedCore = new RegExp('e2e/(app-launch|app-open-close|browser|device-id|device-profile|logger|oidc|storage|use-oidc)', 'i');
console.log('--- unescaped core pattern ---');
for (const p of paths) console.log(p, '=>', unescapedCore.test(p));
"Length of output: 1342
This confirms the analysis: the escaped form doesn't actually create a working alternation in JS RegExp (which is what Jest's --testPathPattern uses under the hood). The pre-existing test:bs:android:core script has the same issue — it doesn't match any of the intended e2e/... files either, so "it has been working" is likely not due to the regex actually selecting those files as intended.
@tsdamas In JavaScript regex (V8/Node, which Jest uses for testPathPattern), \( and \) mean a literal, non-grouping parenthesis character, and \| means a literal pipe character — the opposite of POSIX BRE, where unescaped (/)/| are literal and \(/\)/\| denote groups/alternation. Since Jest's pattern matching uses RegExp.prototype.test, the escaped form e2e/\(app-launch\|...\) only matches a literal string containing literal (, |, ) characters — it won't match any real file path, so it effectively matches nothing.
Given the verification above, both test:bs:android:core and the new test:bs:android:journey-davinci script's pattern don't perform the intended file-selection via alternation. If test:bs:android:core is indeed running its intended targeted subset of tests, it would be worth double-checking how that's invoked in CI (e.g., an env var override, or whether it's actually running all tests unfiltered) rather than relying on the escaped pattern.
To fix test:bs:android:journey-davinci, I'd suggest:
"test:bs:android:journey-davinci": "NODE_NO_WARNINGS=1 detox test --configuration android.bs --loglevel trace --testPathPattern 'e2e/(journey|davinci)'"Would you like me to open a follow-up issue to also fix test:bs:android:core's pattern, since it's pre-existing and out of scope for this PR?
| * this indirection lets DaVinci inject its collector lookup at init time without | ||
| * creating a circular dependency. | ||
| */ | ||
| suspend fun resolveDaVinciCollectors(davinciId: String): List<Any>? = |
There was a problem hiding this comment.
Perhaps add @param davinciId - registered daVinciId
| @@ -0,0 +1,2 @@ | |||
| <manifest xmlns:android="http://schemas.android.com/apk/res/android"> | |||
There was a problem hiding this comment.
If it's empty, this is not required and can be deleted.
| clientId = payload.clientId | ||
| redirectUri = payload.redirectUri | ||
| scopes = payload.scopes.toMutableSet() | ||
| payload.signOutRedirectUri?.let { signOutRedirectUri = it } |
There was a problem hiding this comment.
Would recommend to have these pulled out in an updateOidc config values. For eg: https://github.com/ForgeRock/ping-android-sdk/blob/ef8bc6f21a0e01fa4f9dbc00b6493de1cc8424b0/foundation/oidc/src/main/kotlin/com/pingidentity/oidc/OidcClient.kt#L106
This will also help with Journey and Oidc configs as these values are common.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes