Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis change adds ChangesDaVinci polling and QR collectors
Estimated code review effort: 5 (Critical) | ~120 minutes Mergeability Score: ⚪ Minimal · up to The PR adds DaVinci polling and QR-code collector support with native, TypeScript, sample-app, and test changes; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant DaVinciClient
participant NativeBridge
participant DeviceEventEmitter
participant PollingRenderer
DaVinciClient->>NativeBridge: start pollDaVinci(davinciId, options)
NativeBridge-->>DaVinciClient: return subscription ID
NativeBridge->>DeviceEventEmitter: emit polling status
DeviceEventEmitter-->>DaVinciClient: deliver matching status
DaVinciClient->>PollingRenderer: update polling display
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (71.88%) 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 #62 +/- ##
============================================
+ Coverage 70.81% 71.88% +1.06%
- Complexity 187 194 +7
============================================
Files 193 167 -26
Lines 20297 19951 -346
Branches 887 715 -172
============================================
- Hits 14374 14342 -32
+ Misses 5796 5534 -262
+ Partials 127 75 -52
... and 26 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
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: 12
🧹 Nitpick comments (3)
packages/davinci/android/src/main/java/com/pingidentity/rndavinci/RNPingDavinciEvents.kt (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd KDoc for
POLLING_STATUS.
POLLING_STATUSis a public declaration. Add KDoc directly above the constant.As per coding guidelines: "Use KDoc
/** */on all public and internal 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/davinci/android/src/main/java/com/pingidentity/rndavinci/RNPingDavinciEvents.kt` at line 13, Add a KDoc comment directly above the public POLLING_STATUS constant describing the polling status event, using the required /** */ syntax.Source: Coding guidelines
packages/davinci/ios/Tests/RNPingDavinciCommonTests.swift (1)
1094-1115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStore the observation token to actually remove the block-based observer.
addObserver(forName:object:queue:using:)returns an opaque token.removeObserver(self)does not remove that registration, so eachEventObserverleaves a live observation for the process lifetime. The weakselfcapture keeps behavior correct, but registrations accumulate across tests and every later emission invokes all of them.Keep the returned token and remove it in
deinit.♻️ Proposed refactor
private final class EventObserver: `@unchecked` Sendable { private let lock = NSLock() private var _events: [[String: Any]] = [] + private var token: NSObjectProtocol? var onEvent: (([String: Any]) -> Void)? @@ init() { - NotificationCenter.default.addObserver( + token = NotificationCenter.default.addObserver( forName: .pingDavinciNativeEmit, object: nil, queue: nil ) { [weak self] notification in @@ deinit { - NotificationCenter.default.removeObserver(self) + if let token { + NotificationCenter.default.removeObserver(token) + } } }🤖 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 1094 - 1115, Update EventObserver’s init to store the token returned by NotificationCenter.addObserver(forName:object:queue:using:) in a property, then remove that stored token in deinit instead of self. Preserve the existing weak capture and event-handling behavior.packages/davinci/android/src/main/java/com/pingidentity/rndavinci/RNPingDavinciCommon.kt (1)
554-564: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the nullable
if/elsewith?.let { } ?:.The coding guidelines forbid
if (x != null) ... else ...for nullable handling in Android Kotlin sources. Lines 560-564 use that form onrequestedKey. The option read on lines 554-558 can also usetakeIf/letfor consistency.As per coding guidelines: "Never use
if (x != null) ... else ...— usex?.let { } ?:instead".♻️ Proposed refactor
- val requestedKey = if (options.hasKey("key") && !options.isNull("key")) { - options.getString("key") - } else { - null - } + val requestedKey = options + .takeIf { it.hasKey("key") && !it.isNull("key") } + ?.getString("key") val collectors = node.actions.filterIsInstance<PollingCollector>() - val collector = (if (requestedKey != null) { - collectors.firstOrNull { it.id() == requestedKey } - } else { - collectors.firstOrNull() - }) + val collector = requestedKey + ?.let { key -> collectors.firstOrNull { it.id() == key } } + ?: collectors.firstOrNull()🤖 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 554 - 564, Refactor the nullable handling in the collector selection around requestedKey and the option read: replace the explicit null-check if/else with takeIf/?.let and the Elvis fallback, while preserving the existing behavior of selecting the requested collector or first collector.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 431-438: The polling mapper mapPollingCollector must always emit
numeric pollInterval and pollRetries values. Replace each toIntOrNull fallback
with the deterministic numeric fallback requested and call logWarning when
coercion fails; update DaVinciNodeMapperTest.kt lines 742-765 to assert the
numeric fallback and rename the test to describe numeric behavior.
In
`@packages/davinci/android/src/main/java/com/pingidentity/rndavinci/RNPingDavinciCommon.kt`:
- Around line 606-633: Update emitPollingStatus to be a suspend function and
replace its scope.launch wrapper with a suspend context switch to
Dispatchers.Main before emitting. Ensure callers invoke it from the existing
poll Job so cancellation propagates and queued events are dropped during dispose
or cleanup.
In `@packages/davinci/ios/RNPingDavinciCommon.swift`:
- Around line 508-522: Update the poll bookkeeping around the task creation flow
to key registrations by subscriptionId instead of a Task reference: remove
taskRef, register the task before starting its execution, and have completion
remove using davinciId and subscriptionId. Update PollJobStore’s storage and
register/remove/cancelAll/removeAll methods to manage inner
subscriptionId-to-task entries, with register overwriting defensively so an
early completion cannot reinsert a finished task.
In `@packages/davinci/ios/RNPingDavinciEventEmitterGate.mm`:
- Around line 33-45: Make event-emitter ownership bridge-scoped: update
RNPingDavinciEventEmitterGate.mm so RNPingDavinciClaimEventEmitterOwnership
tracks the active owner and provides synchronized release or handoff, declare
and document that API in RNPingDavinciEventEmitterGate.h, and call it when
removing observers in RNPingDavinci.mm and RNPingDavinciClassic.mm; ensure the
active bridge module can reclaim ownership after teardown or reload. Affected
sites: packages/davinci/ios/RNPingDavinciEventEmitterGate.mm lines 33-45,
packages/davinci/ios/RNPingDavinciEventEmitterGate.h lines 12-20,
packages/davinci/ios/RNPingDavinci.mm lines 35-53, and
packages/davinci/ios/RNPingDavinciClassic.mm lines 47-65.
In `@packages/davinci/ios/RNPingDavinciEvents.swift`:
- Around line 13-21: Document the public constants pollingStatus and
pingDavinciNativeEmit with concise triple-slash comments directly above each
declaration, describing the notification they represent; retain the existing
type and extension comments.
In `@packages/davinci/ios/Tests/RNPingDavinciCommonTests.swift`:
- Around line 785-798: Prevent repeated fulfillment in the event observers used
by the affected tests, including testCleanupCancelsAllOutstandingPollTasks.
Configure each first-event expectation with assertForOverFulfill disabled, or
clear observer.onEvent immediately after the first callback, while preserving
the existing expectation and polling behavior.
In `@packages/davinci/README.md`:
- Around line 336-353: Update the pollStatus README example and its accompanying
guidance to consistently document terminal-status handling: either call next()
for every listed terminal status or state that it applies only to complete. Keep
the example’s switch cases and unsubscribe behavior aligned with the documented
choice.
- Line 334: Update the README example’s client.next call to handle its returned
Promise, ensuring rejected advances are caught and surfaced rather than becoming
unhandled rejections. Preserve the explicit advancement behavior and use the
example’s existing error-handling approach if available.
In `@packages/davinci/src/index.tsx`:
- Around line 42-45: Remove the DaVinciEvents export from the package index so
this internal event-name constant is not exposed as public API. Leave the
internal events module and pollStatus listener usage unchanged.
In `@packages/davinci/src/useDavinci.tsx`:
- Around line 84-97: Update the pollStatus documentation to state that the
returned unsubscribe function only removes the local status listener and does
not cancel the active native poll. Keep the existing promise return type and
active PollingCollector error description unchanged.
In `@PingTestRunner/__tests__/integration/davinci-polling.test.ts`:
- Around line 159-190: Update client.pollStatus and its polling startup flow to
register DeviceEventEmitter listeners before awaiting or initiating pollDaVinci,
ensuring early complete, error, and status events reach onStatus. Modify the
makeMock setup in the “resolves subscriptionId before any tick is delivered”
test so pollDaVinci emits a status before its promise resolves, and assert that
event is received to make the race detectable.
In `@PingTestRunner/ios/PingTestRunnerUITests/DaVinciUITests.swift`:
- Around line 81-87: Update testUserinfoReturnsPayloadContainingSub so the
assertion failure message does not interpolate or otherwise log the full
userinfo payload; retain the check that the payload contains "sub" while using a
non-sensitive failure message.
---
Nitpick comments:
In
`@packages/davinci/android/src/main/java/com/pingidentity/rndavinci/RNPingDavinciCommon.kt`:
- Around line 554-564: Refactor the nullable handling in the collector selection
around requestedKey and the option read: replace the explicit null-check if/else
with takeIf/?.let and the Elvis fallback, while preserving the existing behavior
of selecting the requested collector or first collector.
In
`@packages/davinci/android/src/main/java/com/pingidentity/rndavinci/RNPingDavinciEvents.kt`:
- Line 13: Add a KDoc comment directly above the public POLLING_STATUS constant
describing the polling status event, using the required /** */ syntax.
In `@packages/davinci/ios/Tests/RNPingDavinciCommonTests.swift`:
- Around line 1094-1115: Update EventObserver’s init to store the token returned
by NotificationCenter.addObserver(forName:object:queue:using:) in a property,
then remove that stored token in deinit instead of self. Preserve the existing
weak capture and event-handling behavior.
🪄 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: d80f3ca3-8e9b-480b-882f-1cce72c8b164
⛔ Files ignored due to path filters (2)
PingSampleApp/ios/Podfile.lockis excluded by!**/*.lockPingTestRunner/ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (67)
PingSampleApp/android/app/build.gradlePingSampleApp/src/styles/davinciStyles.tsPingSampleApp/ui/davinci/components/molecules/DaVinciFieldRenderer.tsxPingSampleApp/ui/davinci/components/molecules/DaVinciPollingField.tsxPingSampleApp/ui/davinci/components/molecules/DaVinciQrCodeField.tsxPingSampleApp/ui/davinci/components/molecules/types.tsPingSampleApp/ui/davinci/components/organisms/DaVinciClientPanel.tsxPingSampleApp/ui/davinci/components/organisms/DaVinciContinueNodePanel.tsxPingSampleApp/ui/davinci/hooks/useDaVinciClientPanelController.tsPingTestRunner/__tests__/integration/davinci-polling.test.tsPingTestRunner/android/app/build.gradlePingTestRunner/ios/PingTestRunnerUITests/BaseTestCase.swiftPingTestRunner/ios/PingTestRunnerUITests/DaVinciUITests.swiftPingTestRunner/ios/PingTestRunnerUITests/TestEnvironment.swiftPingTestRunner/scenarios/DaVinciScenario.tsxpackages/binding/android/build.gradlepackages/browser/android/build.gradlepackages/core/android/build.gradlepackages/davinci/README.mdpackages/davinci/RNPingDavinci.podspecpackages/davinci/android/build.gradlepackages/davinci/android/src/main/java/com/pingidentity/rndavinci/RNPingDavinciCommon.ktpackages/davinci/android/src/main/java/com/pingidentity/rndavinci/RNPingDavinciEvents.ktpackages/davinci/android/src/main/java/com/pingidentity/rndavinci/error/DaVinciErrorCodes.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/mapper/DaVinciNodeMapperTest.ktpackages/davinci/ios/Error/DaVinciErrorCodes.swiftpackages/davinci/ios/Mapper/DaVinciNodeMapper.swiftpackages/davinci/ios/RNPingDavinci.mmpackages/davinci/ios/RNPingDavinciClassic.mmpackages/davinci/ios/RNPingDavinciCommon.swiftpackages/davinci/ios/RNPingDavinciEventEmitterGate.hpackages/davinci/ios/RNPingDavinciEventEmitterGate.mmpackages/davinci/ios/RNPingDavinciEvents.swiftpackages/davinci/ios/RNPingDavinciImpl.swiftpackages/davinci/ios/Tests/DaVinciNodeMapperTests.swiftpackages/davinci/ios/Tests/RNPingDavinciCommonTests.swiftpackages/davinci/src/NativeRNPingDavinci.tspackages/davinci/src/__tests__/collectorHelpers.test.tspackages/davinci/src/__tests__/createDaVinciClient.test.tspackages/davinci/src/__tests__/davinciMethods.test.tspackages/davinci/src/__tests__/useDavinci.test.tsxpackages/davinci/src/collectorHelpers.tspackages/davinci/src/davinci.tspackages/davinci/src/davinciMethods.tspackages/davinci/src/events.tspackages/davinci/src/index.tsxpackages/davinci/src/types/client.types.tspackages/davinci/src/types/error.types.tspackages/davinci/src/types/form.types.tspackages/davinci/src/types/node.types.tspackages/davinci/src/useDavinci.tsxpackages/device-client/android/build.gradlepackages/device-id/android/build.gradlepackages/device-profile/android/build.gradlepackages/external-idp/RNPingExternalIdp.podspecpackages/external-idp/android/build.gradlepackages/fido/android/build.gradlepackages/journey/android/build.gradlepackages/logger/android/build.gradlepackages/oath/android/build.gradlepackages/oidc/android/build.gradlepackages/push/android/build.gradlepackages/storage/android/build.gradle
💤 Files with no reviewable changes (2)
- PingSampleApp/android/app/build.gradle
- PingTestRunner/android/app/build.gradle
Comment on lines
+159
to
+190
| it('resolves subscriptionId before any tick is delivered, then streams continue ticks', async () => { | ||
| const mock = makeMock(); | ||
| const { mod, emitter } = await loadDaVinci(mock); | ||
| const client = mod.createDaVinciClient(VALID_CONFIG); | ||
| await client.start(); | ||
|
|
||
| const onStatus = jest.fn(); | ||
| await client.pollStatus(onStatus); | ||
|
|
||
| expect(mock.pollDaVinci).toHaveBeenCalledWith('davinci-id-mock', {}); | ||
|
|
||
| emitter.emit(POLLING_STATUS_EVENT, { | ||
| subscriptionId: 'sub-1', | ||
| status: 'continue', | ||
| retryCount: 1, | ||
| maxRetries: 60, | ||
| }); | ||
| emitter.emit(POLLING_STATUS_EVENT, { | ||
| subscriptionId: 'sub-1', | ||
| status: 'continue', | ||
| retryCount: 2, | ||
| maxRetries: 60, | ||
| }); | ||
|
|
||
| expect(onStatus).toHaveBeenCalledTimes(2); | ||
| expect(onStatus).toHaveBeenNthCalledWith(1, { | ||
| subscriptionId: 'sub-1', | ||
| status: 'continue', | ||
| retryCount: 1, | ||
| maxRetries: 60, | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Prevent loss of the first polling status.
client.pollStatus() waits for pollDaVinci() before it installs DeviceEventEmitter listeners. The iOS bridge starts the polling task before it resolves subscriptionId. A fast complete or error event can occur in that interval and never reach onStatus.
Use a listener-before-start handshake, or buffer native events until JavaScript has subscribed. Update this test so pollDaVinci emits a status before its promise resolves. The current mock cannot detect the race.
🤖 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/__tests__/integration/davinci-polling.test.ts` around lines
159 - 190, Update client.pollStatus and its polling startup flow to register
DeviceEventEmitter listeners before awaiting or initiating pollDaVinci, ensuring
early complete, error, and status events reach onStatus. Modify the makeMock
setup in the “resolves subscriptionId before any tick is delivered” test so
pollDaVinci emits a status before its promise resolves, and assert that event is
received to make the race detectable.
rodrigoareis
approved these changes
Aug 12, 2026
rodrigoareis
left a comment
rodrigoareis
left a comment
Contributor
There was a problem hiding this comment.
Changes looks good to me, left just some minor comments
| pollJobsByDaVinciId[davinciId]?.remove(job) | ||
| } | ||
| } | ||
| pollJobsByDaVinciId.getOrPut(davinciId) { ConcurrentHashMap.newKeySet() }.add(job) |
Contributor
There was a problem hiding this comment.
Suggested change
| pollJobsByDaVinciId.getOrPut(davinciId) { ConcurrentHashMap.newKeySet() }.add(job) | |
| pollJobsByDaVinciId.computeIfAbsent(davinciId) { ConcurrentHashMap.newKeySet() }.add(job) |
| pollJobsByDaVinciId[davinciId]?.remove(job) | ||
| } | ||
| } | ||
| pollJobsByDaVinciId.getOrPut(davinciId) { ConcurrentHashMap.newKeySet() }.add(job) |
Contributor
There was a problem hiding this comment.
Small concurrency nit: getOrPut on a ConcurrentHashMap isn't atomic
| */ | ||
| private fun mapPollingCollector(collector: PollingCollector): Map<String, Any?> { | ||
| val map = baseCollectorMap(collector) | ||
| map["pollInterval"] = collector.pollInterval.toIntOrNull() ?: collector.pollInterval |
Contributor
There was a problem hiding this comment.
Nitpick: when pollInterval/pollRetries fails to parse as Int, we fall back to the raw String. That silently breaks the documented "always a number" contract with the TS side and gives no signal that something's off. Since this mapper already logs warnings elsewhere for unsupported fields (see logWarning usage above), could we do the same here, e.g. logWarning(logger, TAG, "Non-numeric pollInterval from server: $it"), so this doesn't fail silently in production?
…5, SDKS-5296). Add NOTE comments above the affected bridge code so we remember to revisit once upstream ships fixes
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
Adds support for two new DaVinci collector types that drive out-of-band authentication flows: the async
PollingCollector(push approval, QR scan, email verification) and the display-onlyQRCodeCollector. Includes full native bridge implementations on Android and iOS, TypeScript client API, sample app UI, and E2E test coverage.What's new
PollingCollector+pollStatus— New collector type onDaVinciCollector.DaVinciClient.pollStatus()streamsPollingStatusevents (continue|complete|timedOut|expired|error) mirroring iOS'sAsyncStream<PollingStatus>and Android'sFlow<PollingStatus>. Terminal statuses do not auto-advance the flow — callers must callnext()explicitly to progress.QRCodeCollector— Display-only collector (nolabel/required, doesn't extendBaseCollector, matching nativeCollector<Nothing>/Collector). Exposescontent(data URI),fallbackText, and raw server field JSON.pollDaVinci(returns a native subscription id) and supporting event plumbing (RNPingDavinciEvents, iOSRNPingDavinciEventEmitterGate).DaVinciPollingFieldandDaVinciQrCodeFieldmolecules, wired intoDaVinciFieldRenderer/DaVinciContinueNodePanel, plus controller hook updates.DaVinciUITests.swift),BaseTestCase/TestEnvironmenthelpers, and a newdavinci-pollingintegration test suitTesting
RNPingDavinciCommonTest.kt,DaVinciNodeMapperTest.kt(Android);RNPingDavinciCommonTests.swift,DaVinciNodeMapperTests.swift(iOS);collectorHelpers.test.ts,createDaVinciClient.test.ts,davinciMethods.test.ts,useDavinci.test.tsx(TS).davinci-polling.test.tsinPingTestRunner/__tests__/integration/.DaVinciUITests.swiftcovering the new polling/QR scenario inPingTestRunner/scenarios/DaVinciScenario.tsx.Docs
packages/davinci/README.mdupdated with usage forpollStatus,PollingCollector, andQRCodeCollector.Summary by CodeRabbit
New Features
Bug Fixes
Documentation