Conversation
|
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:
📝 WalkthroughWalkthroughAdds a ChangesMetadata collector
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DaVinci
participant CollectorRegistry
participant MetadataCollector
participant Metadata
participant Collectors
DaVinci->>CollectorRegistry: Register METADATA collector
DaVinci->>MetadataCollector: Initialize payload and metadata
MetadataCollector->>Metadata: Render metadata and actions
Metadata->>MetadataCollector: Set success or error result
Metadata->>Collectors: Continue with onNext
Collectors-->>DaVinci: Serialize actionKey and formData
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
davinci/src/main/kotlin/com/pingidentity/davinci/collector/MetadataCollector.kt (1)
48-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare
publicexplicitly on the documented API surface.As per coding guidelines, "Prefer
internalfor module-private APIs, and declarepublicexplicitly on the documented API surface." SinceMetadataCollectorand its properties are intended to be accessed by SDK integrators to retrieve metadata and setoutputorerror, they should explicitly use thepublicmodifier.♻️ Proposed refactor
-class MetadataCollector : Collector<Nothing>, ContinueNodeAware, RequestInterceptor { +public class MetadataCollector : Collector<Nothing>, ContinueNodeAware, RequestInterceptor { override lateinit var continueNode: ContinueNode /** * The key of this field (typically "sdkMetadata"). */ - var key = "" + public var key: String = "" private set /** * The raw type string (always "METADATA"). */ - var type = "" + public var type: String = "" private set /** * The metadata payload sent by the server — arbitrary JSON the SDK must process. */ - var metadata: JsonObject = JsonObject(emptyMap()) + public var metadata: JsonObject = JsonObject(emptyMap()) private set /** * The SDK result to send back on resume (success path). * Set this before calling [ContinueNode.next]. */ - var output: JsonObject? = null + public var output: JsonObject? = null /** * The SDK error to send back on resume (failure path). * When set, [output] is ignored and the flow takes the error branch. * Set this before calling [ContinueNode.next]. */ - var error: JsonObject? = null + public var error: JsonObject? = null🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@davinci/src/main/kotlin/com/pingidentity/davinci/collector/MetadataCollector.kt` around lines 48 - 81, Explicitly declare public visibility on MetadataCollector and its documented API properties: continueNode, key, type, metadata, output, and error. Preserve the existing private setters and nullable types while making the integrator-facing surface unambiguous.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.
Nitpick comments:
In
`@davinci/src/main/kotlin/com/pingidentity/davinci/collector/MetadataCollector.kt`:
- Around line 48-81: Explicitly declare public visibility on MetadataCollector
and its documented API properties: continueNode, key, type, metadata, output,
and error. Preserve the existing private setters and nullable types while making
the integrator-facing surface unambiguous.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 604e6cf3-3844-485a-bc9b-1fcdb85d1a56
📒 Files selected for processing (5)
davinci/src/main/kotlin/com/pingidentity/davinci/CollectorRegistry.ktdavinci/src/main/kotlin/com/pingidentity/davinci/collector/MetadataCollector.ktdavinci/src/test/kotlin/com/pingidentity/davinci/MetadataCollectorTest.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/davinci/collector/DaVinciContinueNode.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/davinci/collector/Metadata.kt
GitHub Actions
Inactive
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #226 +/- ##
==============================
==============================
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
GitHub Actions
Inactive
There was a problem hiding this comment.
🧹 Nitpick comments (2)
davinci/src/main/kotlin/com/pingidentity/davinci/collector/Collectors.kt (1)
62-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce code duplication by lifting
it.payload()?.letoutside thewhenblock.The
it.payload()?.let { ... }check is duplicated across all branches of thewhenstatement. You can improve readability and maintainability by extracting the null-check and restructuring thewhenblock.♻️ Proposed refactor
- when { - it is MetadataCollector -> { - it.payload()?.let { payload -> - put("actionKey", it.id()) - map[it.id()] = payload - } - } - it is SubmitCollector || it is FlowCollector -> { - it.payload()?.let { _ -> - put("actionKey", it.id()) - } - } - else -> { - it.payload()?.let { payload -> - map[it.id()] = payload - } - } - } + it.payload()?.let { payload -> + when (it) { + is MetadataCollector -> { + put("actionKey", it.id()) + map[it.id()] = payload + } + is SubmitCollector, is FlowCollector -> { + put("actionKey", it.id()) + } + else -> { + map[it.id()] = payload + } + } + }🤖 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 `@davinci/src/main/kotlin/com/pingidentity/davinci/collector/Collectors.kt` around lines 62 - 79, Refactor the collector handling around the when expression so the shared it.payload()?.let null-check is performed once outside the branch-specific logic. Inside that block, keep MetadataCollector adding actionKey and storing the payload, SubmitCollector/FlowCollector adding only actionKey, and other collectors storing the payload in map.davinci/src/test/kotlin/com/pingidentity/davinci/MetadataCollectorTest.kt (1)
112-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse consistent and idiomatic JSON boolean assertions.
The test assertions for
isClientErroruse different access patterns (content?.toBoolean()vsboolean ?: true). For consistency and better readability, prefer usingbooleanandassertEquals, which matches the assertion style used on line 176.♻️ Proposed refactor
- val error = collector.payload()!!["error"]?.jsonObject - assertEquals(true, error?.get("isClientError")?.jsonPrimitive?.content?.toBoolean()) - } - - `@Test` - fun setErrorWithIsClientErrorFalseIncludesFlag() { - val collector = MetadataCollector().apply { init(buildFullMetadataJson()) } - collector.setError(errorCode = "E1", message = "m", isClientError = false) - - val error = collector.payload()!!["error"]?.jsonObject - assertFalse(error?.get("isClientError")?.jsonPrimitive?.boolean ?: true) + val error = collector.payload()!!["error"]?.jsonObject + assertEquals(true, error?.get("isClientError")?.jsonPrimitive?.boolean) + } + + `@Test` + fun setErrorWithIsClientErrorFalseIncludesFlag() { + val collector = MetadataCollector().apply { init(buildFullMetadataJson()) } + collector.setError(errorCode = "E1", message = "m", isClientError = false) + + val error = collector.payload()!!["error"]?.jsonObject + assertEquals(false, error?.get("isClientError")?.jsonPrimitive?.boolean)🤖 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 `@davinci/src/test/kotlin/com/pingidentity/davinci/MetadataCollectorTest.kt` around lines 112 - 123, Update the `setErrorWithIsClientErrorFalseIncludesFlag` test assertion to use the JSON primitive’s `boolean` property with `assertEquals`, matching the existing `isClientError` assertion style and the pattern used elsewhere in `MetadataCollectorTest`; remove the fallback-based `assertFalse` form.
🤖 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.
Nitpick comments:
In `@davinci/src/main/kotlin/com/pingidentity/davinci/collector/Collectors.kt`:
- Around line 62-79: Refactor the collector handling around the when expression
so the shared it.payload()?.let null-check is performed once outside the
branch-specific logic. Inside that block, keep MetadataCollector adding
actionKey and storing the payload, SubmitCollector/FlowCollector adding only
actionKey, and other collectors storing the payload in map.
In `@davinci/src/test/kotlin/com/pingidentity/davinci/MetadataCollectorTest.kt`:
- Around line 112-123: Update the `setErrorWithIsClientErrorFalseIncludesFlag`
test assertion to use the JSON primitive’s `boolean` property with
`assertEquals`, matching the existing `isClientError` assertion style and the
pattern used elsewhere in `MetadataCollectorTest`; remove the fallback-based
`assertFalse` form.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ebd23aae-9129-48a3-9d08-cde7f0cd6cb8
📒 Files selected for processing (4)
davinci/src/main/kotlin/com/pingidentity/davinci/collector/Collectors.ktdavinci/src/main/kotlin/com/pingidentity/davinci/collector/MetadataCollector.ktdavinci/src/test/kotlin/com/pingidentity/davinci/MetadataCollectorTest.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/davinci/collector/Metadata.kt
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
davinci/src/main/kotlin/com/pingidentity/davinci/collector/MetadataCollector.kt (1)
61-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the public API visibility explicitly.
Use
public fun setError(...)rather than relying on Kotlin’s default visibility for this documented SDK API.🤖 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 `@davinci/src/main/kotlin/com/pingidentity/davinci/collector/MetadataCollector.kt` at line 61, Update the MetadataCollector.setError method declaration to explicitly use public visibility, preserving its existing parameters and behavior.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
`@davinci/src/main/kotlin/com/pingidentity/davinci/collector/MetadataCollector.kt`:
- Around line 61-68: Update the KDoc and any published contract examples for
MetadataCollector.setError to remove the optional isClientError field and
document that the error payload contains only code and message. Keep the
implementation unchanged.
---
Nitpick comments:
In
`@davinci/src/main/kotlin/com/pingidentity/davinci/collector/MetadataCollector.kt`:
- Line 61: Update the MetadataCollector.setError method declaration to
explicitly use public visibility, preserving its existing parameters and
behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2496c6b0-acfe-4715-9bf8-e85e13dea865
📒 Files selected for processing (3)
davinci/src/main/kotlin/com/pingidentity/davinci/collector/MetadataCollector.ktdavinci/src/test/kotlin/com/pingidentity/davinci/MetadataCollectorTest.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/davinci/collector/Metadata.kt
💤 Files with no reviewable changes (2)
- samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/davinci/collector/Metadata.kt
- davinci/src/test/kotlin/com/pingidentity/davinci/MetadataCollectorTest.kt
GitHub Actions
Inactive
GitHub Actions
Inactive
witrisna
left a comment
witrisna
left a comment
There was a problem hiding this comment.
Can you add an end to end test under DaVinciTest to make sure the actionKey and eventType are changed for MetadataCallback
GitHub Actions
Inactive
|
I think we should also add the E2E test in the |
4 participants
JIRA Ticket
SDKS-5168
Description
Add Metadata collector to support DaVinci callbacks.
Success
{ "id": "4r92vx2x9u", "eventName": "continue", "parameters": { "eventType": "action", "data": { "actionKey": "sdkMetadata", "formData": { "sdkMetadata": { "status": "success" } } } } }Error
{ "id": "4r92vx2x9u", "eventName": "continue", "parameters": { "eventType": "action", "data": { "actionKey": "sdkMetadata", "formData": { "sdkMetadata": { "error": { "code": "100", "message": "An error occurred" } } } } } }Summary by CodeRabbit
formData.