feat(davinci-client): add metadata collector (SDKS-5100) - #727
Conversation
🦋 Changeset detectedLatest commit: fc0caf2 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 |
|
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 support
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
|
View your CI Pipeline Execution ↗ for commit 44abe1c
💡 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.
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.26%) 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.26% +5.18%
==========================================
Files 155 161 +6
Lines 24398 25661 +1263
Branches 1203 1626 +423
==========================================
+ Hits 4410 5970 +1560
+ Misses 19988 19691 -297
🚀 New features to boost your workflow:
|
|
Deployed e7bf2cf to https://ForgeRock.github.io/ping-javascript-sdk/pr-727/e7bf2cf489baa697bd08ebd2a115c3f77f51d71d branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/journey-client - 92.6 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 |
fdb36d4 to
82986b0
Compare
82986b0 to
c3b91d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/davinci-client/src/lib/client.types.ts`:
- Around line 54-63: Add Record<string, unknown> to the DaVinciRequestValueTypes
union in packages/davinci-client/src/lib/client.types.ts (lines 54-63) so
metadata object values are supported. Regenerate the API report at
packages/davinci-client/api-report/davinci-client.types.api.md (lines 642-643)
to reflect the updated public type.
🪄 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: 1b3d99c0-9fb5-4548-b70b-079d37d802fd
📒 Files selected for processing (18)
.changeset/real-swans-leave.mde2e/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
🚧 Files skipped from review as they are similar to previous changes (12)
- .changeset/real-swans-leave.md
- packages/davinci-client/src/lib/node.reducer.test.ts
- e2e/davinci-app/main.ts
- packages/davinci-client/src/lib/node.types.ts
- packages/davinci-client/src/lib/davinci.api.ts
- packages/davinci-client/src/lib/davinci.types.ts
- packages/davinci-client/src/lib/node.types.test-d.ts
- packages/davinci-client/src/lib/node.slice.ts
- packages/davinci-client/src/lib/client.store.ts
- packages/davinci-client/src/lib/node.reducer.ts
- packages/davinci-client/src/lib/davinci.utils.ts
- packages/davinci-client/api-report/davinci-client.api.md
|
Moved MetadataCollector interface from ObjectValueCollector to ObjectValueAutoCollector category |
c3b91d2 to
8693a1d
Compare
| const formData = collectors?.reduce<{ | ||
| [key: string]: DaVinciRequestValueTypes | Record<string, unknown>; | ||
| }>((acc, collector) => { | ||
| acc[collector.input.key] = collector.input.value; | ||
| return acc; | ||
| }, {}); |
There was a problem hiding this comment.
Besides the MetadataCollector, an action request really shouldn't have any data that is collected but including this here just in case.
| data: { | ||
| actionKey: action, | ||
| actionKey: action || node.client?.action || '', | ||
| ...(Object.keys(formData ?? {}).length && { formData: formData }), |
There was a problem hiding this comment.
Note that in the FIDO error PR coming up this will be updated to formData: formData ?? {} to align with the contract. Native always sends an empty object for all action requests so this should be harmless.
#728
8693a1d to
a2cae19
Compare
|
Added e2e tests |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/davinci-app/components/metadata.ts`:
- Around line 26-35: Update the successBtn.onclick and errorBtn.onclick handlers
to catch rejected submitForm() promises and route the failure through the
example app’s existing error handling or reporting mechanism, while preserving
their current updater calls and submission 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 959fa4f0-82c6-459f-8a41-f8926820dcad
📒 Files selected for processing (20)
.changeset/real-swans-leave.mde2e/davinci-app/components/metadata.tse2e/davinci-app/main.tse2e/davinci-app/server-configs.tse2e/davinci-suites/src/metadata.test.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
🚧 Files skipped from review as they are similar to previous changes (17)
- .changeset/real-swans-leave.md
- packages/davinci-client/src/lib/node.types.test-d.ts
- packages/davinci-client/src/lib/node.reducer.test.ts
- packages/davinci-client/src/lib/node.slice.ts
- packages/davinci-client/src/lib/davinci.api.ts
- packages/davinci-client/src/lib/node.types.ts
- packages/davinci-client/src/lib/davinci.types.ts
- packages/davinci-client/src/lib/collector.types.ts
- e2e/davinci-app/main.ts
- packages/davinci-client/src/lib/collector.utils.test.ts
- packages/davinci-client/src/lib/client.store.ts
- packages/davinci-client/src/lib/collector.utils.ts
- packages/davinci-client/src/lib/node.reducer.ts
- packages/davinci-client/src/lib/davinci.utils.ts
- packages/davinci-client/src/lib/client.types.ts
- packages/davinci-client/api-report/davinci-client.api.md
- packages/davinci-client/api-report/davinci-client.types.api.md
| successBtn.onclick = async () => { | ||
| updater({ status: 'succeeded' }); | ||
| await submitForm(); | ||
| }; | ||
|
|
||
| errorBtn.onclick = async () => { | ||
| const metadataError = getMetadataError({ code: 'ERROR_CODE', message: 'Operation cancelled' }); | ||
| updater(metadataError); | ||
| await submitForm(); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle rejected submission promises.
If submitForm() rejects, both async event handlers produce an unhandled promise rejection, leaving the UI without an error path. Catch the failure and route it through the example app’s existing error handling or reporting mechanism.
🤖 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 `@e2e/davinci-app/components/metadata.ts` around lines 26 - 35, Update the
successBtn.onclick and errorBtn.onclick handlers to catch rejected submitForm()
promises and route the failure through the example app’s existing error handling
or reporting mechanism, while preserving their current updater calls and
submission behavior.
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
This looks good. I just have the one question about the error method.
| * @param {MetadataError} errorDetails - An error code and description. | ||
| * @returns {MetadataError} The structured error object. | ||
| */ | ||
| getMetadataError: (errorDetails: MetadataError): MetadataError => { |
There was a problem hiding this comment.
Are we exposing this in order for the app to conveniently construct the expected error shape if there's a client error? I'm quite hesitant to attach this to the Client store as it's not "interacting" with the store in any meaningful way.
If we want to provide such utility functions, I'd rather export them separately as just functions under a different directory. Maybe something like this:
import { someUtilFn } from '@forgerock/davinci-client/utils';
There was a problem hiding this comment.
Yeah it's a little silly but idk how else the consumer is supposed to know what shape the error needs to be. I wasn't sure where to put this but I can certainly move it to utils.
a2cae19 to
00388a5
Compare
| /** | ||
| * @method getMetadataError - Constructs a structured error object from a code and message. | ||
| * @param {MetadataError} errorDetails - An error code and description. | ||
| * @returns {MetadataError} The structured error object. | ||
| */ | ||
| getMetadataError: (errorDetails: MetadataError): MetadataError => { | ||
| return { | ||
| code: errorDetails.code, | ||
| message: errorDetails.message, | ||
| }; | ||
| }, | ||
|
|
There was a problem hiding this comment.
I would expect this to be on the collector itself, and not part of the public api.
Is there a reason we decided to add it here?
| input: { | ||
| key: field.key, | ||
| value: defaultValue, | ||
| value: field.type !== 'METADATA' ? defaultValue : {}, |
There was a problem hiding this comment.
Do we need this here if we are mutating defaultValue on lines 875/876?
I think we can choose one?
There was a problem hiding this comment.
I think you may have reviewed this before I moved the collector to AutoCollectors. This is outdated.
| export function getMetadataError(errorDetails: MetadataError): MetadataError { | ||
| return { | ||
| code: errorDetails.code, | ||
| message: errorDetails.message, | ||
| }; |
There was a problem hiding this comment.
I spoke with AJ about this. My feeling is that we are creating an API, and a new API surface in general, to do dot notation.
We are already asking the consumer to transform their error shape into a MetadataError and then we are saying we will look up code and message for you.
I don't really understand this abstraction since they've already done the transformation.
It could be a type but then AJ brought up "what if they don't use typescript" which is a fair point, but then the same question arises with how do they know how to get their error shape to MetadataError.
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
Just one major comment about exposing DaVinciRequestValueTypes as public API, rest are very minor comments
| * of the MIT license. See the LICENSE file for details. | ||
| */ | ||
|
|
||
| export interface MetadataError { |
There was a problem hiding this comment.
Is it better to move this to the collector.types.ts or types.ts file or really any other existing types file? Because this is a collector related type, I am wondering the need for a new file here.
There was a problem hiding this comment.
Once we move MetadataError to collector.types.ts file, both utils.ts here and the utils.types.ts files are redundant
| /** | ||
| * Allowed value types for DaVinci formData request bodies. This differs from `CollectorValueTypes` because input values may be transformed for DaVinci. | ||
| */ | ||
| export type DaVinciRequestValueTypes = |
There was a problem hiding this comment.
My understanding is that client.types.ts is for public API, is that correct? This is a type that sends a request to davinci, right? Why is this a public API? Does it belong ing davinci.types.ts file or somewhere non-public?
| export type { FidoClient } from './lib/fido/fido.js'; | ||
|
|
||
| // Utility types | ||
| export * from './lib/utils/utils.types.js'; |
There was a problem hiding this comment.
And this can be removed as well, once the file is gone.
| ".": "./dist/src/index.js", | ||
| "./types": "./dist/src/types.d.ts" | ||
| "./types": "./dist/src/types.d.ts", | ||
| "./utils": "./dist/src/lib/utils/utils.js" |
There was a problem hiding this comment.
This will be unnecessary too once we remove the utils.ts file
00388a5 to
44abe1c
Compare
44abe1c to
fc0caf2
Compare
|
Removed metadata utils. Ready for review again. |
There was a problem hiding this comment.
Nx Cloud has identified a flaky task in your failed CI:
🔂 Since the failure was identified as flaky, we triggered a CI rerun by adding an empty commit to this branch.
🔔 Heads up, your workspace has pending recommendations ↗ to auto-apply fixes for similar failures.
🎓 Learn more about Self-Healing CI on nx.dev
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
Code looks good now! Less files and no public API additions!
Checked the e2e app and both success and failure cases echo back the info!
6 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