feat(davinci-client): add metadata collector (SDKS-5100)#727
Conversation
🦋 Changeset detectedLatest commit: b8c285f The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds ChangesMetadata collector support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant metadataComponent
participant davinciClient
participant DaVinciAPI
User->>metadataComponent: Click metadata success or error
metadataComponent->>davinciClient: Update MetadataCollector
metadataComponent->>davinciClient: Submit form
davinciClient->>DaVinciAPI: Build metadata action request
DaVinciAPI-->>davinciClient: Return next node
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
|
View your CI Pipeline Execution ↗ for commit b8c285f
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/davinci-client/src/lib/client.store.ts (1)
459-471: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove commented-out code.
It looks like the
isClientErrorproperty was considered but isn't part of theMetadataErrortype. Consider removing the dead code to keep the file clean.♻️ Proposed refactor
/** * `@method` getMetadataError - Constructs a structured error object from a code and message. * `@param` {{ code: string; message: string; }} errorDetails - An error code and description. * `@returns` {{ code: string; message: string }} The structured error object. */ - getMetadataError: (errorDetails: MetadataError): MetadataError => { - return { - code: errorDetails.code, - message: errorDetails.message, - // isClientError: true, - }; - }, + getMetadataError: (errorDetails: MetadataError): MetadataError => ({ + code: errorDetails.code, + message: errorDetails.message, + }),🤖 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-client/src/lib/client.store.ts` around lines 459 - 471, Remove the commented-out isClientError line from getMetadataError while preserving the existing MetadataError fields and return behavior.packages/davinci-client/src/lib/node.slice.ts (1)
195-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
findoverfilter()[0].Using
findis slightly more efficient and idiomatic since it avoids allocating an intermediate array and stops iterating once a match is found.♻️ Proposed refactor
- const metadataCollector = collectors.filter( + const metadataCollector = collectors.find( (collector): collector is MetadataCollector => collector.type === 'MetadataCollector', - )[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-client/src/lib/node.slice.ts` around lines 195 - 198, Update the metadata collector lookup in the surrounding node slice logic to use find with the existing MetadataCollector type guard instead of filter()[0]. Preserve the current result, including undefined when no matching collector exists.
🤖 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-client/src/lib/client.types.ts`:
- Around line 47-48: Update the CollectorValueTypes union in client.types.ts to
include Record<string, unknown> alongside MetadataError and the existing value
types, allowing standard object payloads for MetadataCollector updates.
In `@packages/davinci-client/src/lib/node.reducer.ts`:
- Around line 341-347: Update the MetadataCollector validation in the reducer to
explicitly reject null values in addition to non-objects and arrays before
assigning collector.input.value. Also harden the PhoneNumberCollector and
FidoRegistrationCollector validation paths against null before using the in
operator, preserving their existing validation behavior for valid objects.
---
Nitpick comments:
In `@packages/davinci-client/src/lib/client.store.ts`:
- Around line 459-471: Remove the commented-out isClientError line from
getMetadataError while preserving the existing MetadataError fields and return
behavior.
In `@packages/davinci-client/src/lib/node.slice.ts`:
- Around line 195-198: Update the metadata collector lookup in the surrounding
node slice logic to use find with the existing MetadataCollector type guard
instead of filter()[0]. Preserve the current result, including undefined when no
matching collector exists.
🪄 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: e7a845e5-8d09-49fc-ae39-e9e2f576e8e7
📒 Files selected for processing (17)
e2e/davinci-app/components/metadata.tse2e/davinci-app/main.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/client.types.tspackages/davinci-client/src/lib/collector.types.tspackages/davinci-client/src/lib/collector.utils.test.tspackages/davinci-client/src/lib/collector.utils.tspackages/davinci-client/src/lib/davinci.api.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/lib/davinci.utils.tspackages/davinci-client/src/lib/node.reducer.test.tspackages/davinci-client/src/lib/node.reducer.tspackages/davinci-client/src/lib/node.slice.tspackages/davinci-client/src/lib/node.types.test-d.tspackages/davinci-client/src/lib/node.types.ts
| | FidoAuthenticationInputValue | ||
| | MetadataError; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add Record<string, unknown> to CollectorValueTypes.
The CollectorValueTypes union defines the acceptable payloads for the updateCollectorValues action. Since the new MetadataCollector stores input values typed as Record<string, unknown>, this type must be explicitly included in the union alongside MetadataError. Without it, dispatching updates for a metadata collector using a standard object payload can result in TypeScript compilation errors.
💻 Proposed fix
- | FidoAuthenticationInputValue
- | MetadataError;
+ | FidoAuthenticationInputValue
+ | MetadataError
+ | Record<string, unknown>;📝 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.
| | FidoAuthenticationInputValue | |
| | MetadataError; | |
| | FidoAuthenticationInputValue | |
| | MetadataError | |
| | Record<string, unknown>; |
🤖 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-client/src/lib/client.types.ts` around lines 47 - 48, Update
the CollectorValueTypes union in client.types.ts to include Record<string,
unknown> alongside MetadataError and the existing value types, allowing standard
object payloads for MetadataCollector updates.
|
|
||
| if (collector.type === 'MetadataCollector') { | ||
| if (typeof action.payload.value !== 'object' || Array.isArray(action.payload.value)) { | ||
| throw new Error('Value argument must be an object'); | ||
| } | ||
| collector.input.value = action.payload.value as Record<string, unknown>; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent null values from bypassing the object validation.
In JavaScript, typeof null evaluates to 'object'. A null payload value will pass this validation check and be incorrectly assigned to collector.input.value as a Record<string, unknown>, which could cause native TypeErrors when downstream logic attempts to iterate over or spread the object.
Add an explicit null check. (Note: Other collectors like PhoneNumberCollector and FidoRegistrationCollector earlier in this reducer share a similar missing null check before using the in operator; consider hardening those as well).
🛡️ Proposed fix
if (collector.type === 'MetadataCollector') {
- if (typeof action.payload.value !== 'object' || Array.isArray(action.payload.value)) {
+ if (typeof action.payload.value !== 'object' || action.payload.value === null || Array.isArray(action.payload.value)) {
throw new Error('Value argument must be an object');
}
collector.input.value = action.payload.value as Record<string, unknown>;
}📝 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.
| if (collector.type === 'MetadataCollector') { | |
| if (typeof action.payload.value !== 'object' || Array.isArray(action.payload.value)) { | |
| throw new Error('Value argument must be an object'); | |
| } | |
| collector.input.value = action.payload.value as Record<string, unknown>; | |
| } | |
| if (collector.type === 'MetadataCollector') { | |
| if (typeof action.payload.value !== 'object' || action.payload.value === null || Array.isArray(action.payload.value)) { | |
| throw new Error('Value argument must be an object'); | |
| } | |
| collector.input.value = action.payload.value as Record<string, unknown>; | |
| } |
🤖 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-client/src/lib/node.reducer.ts` around lines 341 - 347,
Update the MetadataCollector validation in the reducer to explicitly reject null
values in addition to non-objects and arrays before assigning
collector.input.value. Also harden the PhoneNumberCollector and
FidoRegistrationCollector validation paths against null before using the in
operator, preserving their existing validation behavior for valid objects.
There was a problem hiding this comment.
may be valid if null can sneak in here. perhaps a !== null
There was a problem hiding this comment.
@ryanbas21, agreed. null can reach this runtime path and passes typeof value === 'object'; the condition should explicitly reject it:
if (
typeof action.payload.value !== 'object' ||
action.payload.value === null ||
Array.isArray(action.payload.value)
) {
throw new Error('Value argument must be an object');
}That preserves valid record payloads while rejecting null and arrays.
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We updated the transformActionRequest test to include formData: {} in the expected request payload, matching the new behavior introduced by this PR. The PR modified transformActionRequest in davinci.utils.ts to reduce non-action collectors into a formData object and include it in the request, but the test's expected value was not updated accordingly. This fix aligns the test with the current implementation so the assertion passes correctly.
Tip
✅ We verified this fix by re-running @forgerock/davinci-client:test.
diff --git a/packages/davinci-client/src/lib/davinci.utils.test.ts b/packages/davinci-client/src/lib/davinci.utils.test.ts
index 094d4d9..f2f36eb 100644
--- a/packages/davinci-client/src/lib/davinci.utils.test.ts
+++ b/packages/davinci-client/src/lib/davinci.utils.test.ts
@@ -243,6 +243,7 @@ describe('transformActionRequest', () => {
eventType: 'action',
data: {
actionKey: 'TEST_ACTION',
+ formData: {},
},
},
};
Or Apply changes locally with:
npx nx-cloud apply-locally nPN3-s8Wg
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
d076641 to
b8c285f
Compare
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (23.20%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #727 +/- ##
==========================================
+ Coverage 18.07% 23.20% +5.12%
==========================================
Files 155 161 +6
Lines 24398 25645 +1247
Branches 1203 1624 +421
==========================================
+ Hits 4410 5950 +1540
+ Misses 19988 19695 -293
🚀 New features to boost your workflow:
|
|
Deployed 4b804d1 to https://ForgeRock.github.io/ping-javascript-sdk/pr-727/4b804d1288f8b88912a04e24ee66ecdc394a5b75 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/device-client - 10.0 KB (new) 📊 Minor Changes📈 @forgerock/davinci-client - 55.2 KB (+0.8 KB) ➖ No Changes➖ @forgerock/sdk-types - 9.1 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
3 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-5100
Description
What
Adds
MetadataCollectorsupport todavinci-client, enabling DaVinci flows to deliver server-side metadata payloads (e.g. session tokens, user context) to client applications. The collector carries its payload inoutput.valueand reports success or structured errors back to the server viatransformActionRequest.Why
DaVinci flows need a mechanism to push arbitrary metadata to the client and receive a status (success or error) in return, without relying on a standard form submission. This fills that gap in the collector model.
Changes
MetadataCollector,MetadataError,MetadataField, andDaVinciRequestValueTypes(extracted from inline union indavinci.utils.ts)collector.types.ts/collector.utils.ts: AddedMetadataCollectortype alias andreturnMetadataCollectorfactory; updatedreturnObjectCollectorto handle the label-lessMETADATAfield shapenode.reducer.ts: AddedMETADATAcase to build aMetadataCollectorfrom a field; added object-type guard for MetadataCollector update valuesnode.slice.ts: Falls back tometadataCollector.output.keyforclient.actionwhen noSubmitCollectoris presentdavinci.api.ts: Auto-routes totransformActionRequestwhen aMetadataCollectoris in the collector listdavinci.utils.ts:transformActionRequestnow collects form data from all value-bearing collectors and includes it in the request body alongsideactionKeyclient.store.ts: ExposesgetMetadataErrorhelper on the client facade for constructing structured error objectsreturnMetadataCollectorand reducer MetadataField handling (create, update, invalid update)metadataComponentdemonstrating success and error paths; wired intomain.tsSummary by CodeRabbit
New Features
Bug Fixes
Tests