feat(davinci-client): add metadata collector (SDKS-5100)#727
Conversation
🦋 Changeset detectedLatest commit: 00388a5 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:
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 (26)
🚧 Files skipped from review as they are similar to previous changes (17)
📝 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 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 00388a5
💡 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}
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.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.19%
==========================================
Files 155 165 +10
Lines 24398 25674 +1276
Branches 1203 1630 +427
==========================================
+ Hits 4410 5974 +1564
+ Misses 19988 19700 -288
🚀 New features to boost your workflow:
|
|
Deployed b015428 to https://ForgeRock.github.io/ping-javascript-sdk/pr-727/b0154288625c365329d5318da3ccc689ebb70513 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔺 @forgerock/davinci-client - 56.7 KB (+2.4 KB, +4.3%) 🆕 New Packages🆕 @forgerock/journey-client - 92.6 KB (new) ➖ 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
5 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