feat(davinci-client): add form image collector#698
Conversation
🦋 Changeset detectedLatest commit: 8140a51 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 ChangesImageCollector Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 5aa68e5
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
![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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
vatsalparikh
force-pushed
the
SDKS-5101
branch
2 times, most recently
from
b7a8b7f to
b0d790d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
@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: |
|
Deployed 01d0610 to https://ForgeRock.github.io/ping-javascript-sdk/pr-698/01d0610fd1ae3a30405ee9199e6a6ceb17808602 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/device-client - 0.0 KB (-10.0 KB, -100.0%) 📊 Minor Changes📈 @forgerock/davinci-client - 54.5 KB (+0.7 KB) ➖ No Changes➖ @forgerock/oidc-client - 35.2 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (22.11%) 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 #698 +/- ##
==========================================
+ Coverage 18.07% 22.11% +4.03%
==========================================
Files 155 157 +2
Lines 24398 25230 +832
Branches 1203 1490 +287
==========================================
+ Hits 4410 5579 +1169
+ Misses 19988 19651 -337
🚀 New features to boost your workflow:
|
vatsalparikh
requested review from
SteinGabriel,
ancheetah,
cerebrl and
ryanbas21
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/davinci-client/src/lib/collector.types.test-d.ts (1)
658-672: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a companion type test without
hyperlinkUrlto lock optional semantics.Current coverage validates the “present” case only. Adding a second
ImageCollectorsample withouthyperlinkUrlwould protect against accidentally making it required later.🤖 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/collector.types.test-d.ts` around lines 658 - 672, Add a second type test case for ImageCollector that omits the hyperlinkUrl property from the output object. Create a new test (likely another it block) that validates the InferNoValueCollectorType type inference for ImageCollector but with a minimal output structure that excludes hyperlinkUrl, ensuring the type definition correctly treats hyperlinkUrl as optional and prevents accidental breaking changes that might make it required.packages/davinci-client/src/lib/collector.types.ts (1)
640-649: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAlign
ImageCollectordocs with the declared requireddescriptionfield.The JSDoc says
descriptionis optional, butoutput.descriptionis typed as required (string). Please make the comment consistent with the type contract.Suggested doc-only fix
- * hyperlink URL (`hyperlink.url`), and an optional `description` (alt text). + * hyperlink URL (`hyperlink.url`), and `description` (alt text).🤖 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/collector.types.ts` around lines 640 - 649, The JSDoc comment for the `ImageCollector` interface incorrectly describes `description` as an optional field when the type definition shows `description: string` as required (non-nullable). Update the comment above the `ImageCollector` interface to remove the "optional" designation for `description` and accurately reflect that it is a required field in the output type, while keeping the reference to it being alt text.
🤖 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-suites/src/form-image.test.ts`:
- Around line 22-25: The test assertion for the formImage src attribute uses a
brittle full URL equality check that will fail if query parameters, their order,
or optimization values change. Instead of checking the entire URL with
toHaveAttribute, use a more flexible assertion that checks if the src contains
or matches just the essential parts of the URL (such as the base image path from
destinationcanada.com without the query parameters) to make the test more
resilient to parameter changes.
---
Nitpick comments:
In `@packages/davinci-client/src/lib/collector.types.test-d.ts`:
- Around line 658-672: Add a second type test case for ImageCollector that omits
the hyperlinkUrl property from the output object. Create a new test (likely
another it block) that validates the InferNoValueCollectorType type inference
for ImageCollector but with a minimal output structure that excludes
hyperlinkUrl, ensuring the type definition correctly treats hyperlinkUrl as
optional and prevents accidental breaking changes that might make it required.
In `@packages/davinci-client/src/lib/collector.types.ts`:
- Around line 640-649: The JSDoc comment for the `ImageCollector` interface
incorrectly describes `description` as an optional field when the type
definition shows `description: string` as required (non-nullable). Update the
comment above the `ImageCollector` interface to remove the "optional"
designation for `description` and accurately reflect that it is a required field
in the output type, while keeping the reference to it being alt text.
🪄 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: d0ed34c7-4971-4ef6-b40c-f2162503f81c
📒 Files selected for processing (17)
.changeset/curly-wolves-swim.mde2e/davinci-app/components/form-image.tse2e/davinci-app/main.tse2e/davinci-app/server-configs.tse2e/davinci-suites/src/form-fields.test.tse2e/davinci-suites/src/form-image.test.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/collector.types.test-d.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.types.tspackages/davinci-client/src/lib/node.reducer.test.tspackages/davinci-client/src/lib/node.reducer.tspackages/davinci-client/src/lib/node.types.test-d.tspackages/davinci-client/src/lib/node.types.ts
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
I'd like to simplify the property names, but otherwise, looks good.
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
Lgtm, some minor comments and suggestions
|
@vatsalparikh One more question, should this have a |
Is there a reason to hold back ImageCollector feature in 2.1 release? |
I'm not sure. It's a question for product wether or not it should go out. I would think if we are starting QA, we should be in code freeze. |
vatsalparikh
force-pushed
the
SDKS-5101
branch
2 times, most recently
from
0f1d875 to
6d4382b
Compare
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 reverted the waitForRequest URL filter in form-fields.test.ts from showForm back to customForm, as the DaVinci API's form submission POST request URL contains customForm, not showForm. The showForm filter caused the waitForRequest promise to never resolve, resulting in a 30-second timeout. This fix restores the correct URL matching so the test can proceed after the Submit button is clicked.
Tip
✅ We verified this fix by re-running @forgerock/davinci-suites:e2e-ci--src/form-fields.test.ts.
diff --git a/e2e/davinci-suites/src/form-fields.test.ts b/e2e/davinci-suites/src/form-fields.test.ts
index 101a3e9..c58d792 100644
--- a/e2e/davinci-suites/src/form-fields.test.ts
+++ b/e2e/davinci-suites/src/form-fields.test.ts
@@ -71,7 +71,7 @@ test('Should render form fields', async ({ page }) => {
await expect(page.getByRole('button', { name: 'Flow Link' })).toBeVisible();
const requestPromise = page.waitForRequest(
- (request) => request.url().includes('showForm') && request.method() === 'POST',
+ (request) => request.url().includes('customForm') && request.method() === 'POST',
);
await page.getByRole('button', { name: 'Submit' }).click();
Or Apply changes locally with:
npx nx-cloud apply-locally rp64-yw5H
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
vatsalparikh
force-pushed
the
SDKS-5101
branch
2 times, most recently
from
e0cdac0 to
3de5f3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/collector.utils.ts`:
- Line 1009: The `alt` property assignment on line 1009 uses `field.description`
directly without a fallback value, while the `label` property on line 1007 uses
`field.description ?? ''` to provide an empty string default. This inconsistency
means `alt` will be `undefined` when `field.description` is missing, while
`label` will be an empty string. Apply the same defensive null coalescing
pattern to the `alt` assignment to use an empty string fallback when
`field.description` is absent, ensuring consistent behavior between both
properties.
- Line 1008: The `src` property assignment using `field.imageUrl` lacks a
fallback mechanism for when imageUrl is null or undefined, resulting in invalid
src attributes. Add a fallback value to the `src: field.imageUrl` assignment by
using the pattern `src: field.imageUrl || ''` to match the approach used in the
`returnQrCodeCollector` function, ensuring the image element always has a valid
(even if empty) src attribute.
- Line 1004: The returnImageCollector function explicitly sets error: null,
which overrides validation errors from the base returnNoValueCollector, creating
inconsistency with other collectors like returnQrCodeCollector and
returnReadOnlyCollector that inherit the base error state. Either remove the
explicit error: null assignment from returnImageCollector to inherit the base
validation behavior like the other collectors do, or keep it and add a clear
comment documenting why IMAGE collectors specifically need to bypass validation
error checking while other collectors do not.
🪄 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: b30cfd02-44b0-47b4-bd27-24e01a2e20d8
📒 Files selected for processing (10)
.changeset/curly-wolves-swim.mde2e/davinci-app/components/form-image.tse2e/davinci-suites/src/form-image.test.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/collector.types.test-d.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/node.reducer.test.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/curly-wolves-swim.md
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/davinci-client/src/lib/collector.types.test-d.ts
- e2e/davinci-suites/src/form-image.test.ts
- packages/davinci-client/src/lib/node.reducer.test.ts
- e2e/davinci-app/components/form-image.ts
- packages/davinci-client/src/lib/collector.types.ts
- packages/davinci-client/src/lib/collector.utils.test.ts
- packages/davinci-client/api-report/davinci-client.api.md
- packages/davinci-client/api-report/davinci-client.types.api.md
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
I have one more request regarding error handling but otherwise looks good
| if (!('content' in field)) { | ||
| error = `${error}Content is not found in the field object. `; | ||
| } | ||
| if (!('type' in field)) { | ||
| error = `${error}Type is not found in the field object. `; | ||
| } |
There was a problem hiding this comment.
@vatsalparikh instead of forcing error: null in returnImageCollector I think we should handle the errors here. If there is no type in field then the collector should have an error but it will be overridden in returnImageCollector so this is a bug. We can do something like if (field.type !== 'IMAGE') { // content error }
There was a problem hiding this comment.
There are two types of errors we are checking for in NoValueCollector, content and type. content property does not exist on ImageCollector.
However, I do see how errors can be swallowed. If we were to ever extend those checks in NoValueCollector and they are relevant for ImageCollector, those errors will be swallowed.
So I believe the bug is non-existent at the moment, and in fact, inheriting the error would mean that we are saying image collector has no content, when in reality ImageCollector will never have the content property. This is what is expected in ImageCollector
{
"type": "IMAGE",
"key": "<field key>",
"description": "<alt text>",
"imageUrl": "<absolute URL>",
"hyperlinkUrl": "<absolute URL>"
}
However, you raised something interesting. Now that content is a field only applicable for ReadOnlyCollector and QrCodeCollector, I am thinking we can do those checks for individual collectors rather than doing it for the base collector. This way we can extend the errors that NoValueCollector exposes and avoid hardcoding error as null in ImageCollector.
SteinGabriel
left a comment
SteinGabriel
left a comment
There was a problem hiding this comment.
Minor finding.
Also, copyright header needs to be updated in a few files:
node.reducer.tsclient.types.tsnode.types.tsnode.types.test-d.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/davinci-client/src/lib/collector.types.ts (1)
584-590: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAgreementCollector instances will inherit a false error state.
returnAgreementCollector()currently reusesreturnNoValueCollector()without clearingerror, and that base builder flags any read-only field that lackscontent. Since the agreement path is populated fromtitle/titleEnabled/agreement/enabledinstead, validAGREEMENTfields will be exposed witherror !== nullon creation.returnImageCollector()already handles the same base-builder mismatch by resettingerrortonull.Also applies to: 712-718
🤖 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/collector.types.ts` around lines 584 - 590, `returnAgreementCollector()` is inheriting the false validation state from `returnNoValueCollector()`, so AGREEMENT fields are created with a non-null `error` even when valid. Update the `returnAgreementCollector` builder to clear `error` after reusing the base collector, following the same pattern used in `returnImageCollector`, and keep the AGREEMENT-specific fields (`title`, `titleEnabled`, `agreement`, `enabled`) as the source of truth.packages/davinci-client/src/lib/node.reducer.ts (1)
144-145: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftThis silently changes the public contract for non-required
SINGLE_CHECKBOXfields.Line 145 now emits
ValidatedBooleanCollectorfor every checkbox, so flows that previously receivedBooleanCollectorforrequired: falsewill stop matching on the oldtype/category. The updated reducer test also codifies that new behavior for the non-required case, so this is an SDK-breaking change rather than just an internal refactor. Please either preserve the legacy constructor for non-required fields or ship this as an explicit breaking change tied to the boolean collector type removals.🤖 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 144 - 145, The SINGLE_CHECKBOX branch in node.reducer.ts is changing the public contract for non-required fields by always returning ValidatedBooleanCollector, which breaks callers that still expect BooleanCollector for required: false cases. Update the return logic in the reducer path for SINGLE_CHECKBOX so non-required fields keep the legacy BooleanCollector behavior, or explicitly gate this as a breaking change by aligning the reducer, returnValidatedBooleanCollector, and the related tests/collector type removals together.
🤖 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.
Outside diff comments:
In `@packages/davinci-client/src/lib/collector.types.ts`:
- Around line 584-590: `returnAgreementCollector()` is inheriting the false
validation state from `returnNoValueCollector()`, so AGREEMENT fields are
created with a non-null `error` even when valid. Update the
`returnAgreementCollector` builder to clear `error` after reusing the base
collector, following the same pattern used in `returnImageCollector`, and keep
the AGREEMENT-specific fields (`title`, `titleEnabled`, `agreement`, `enabled`)
as the source of truth.
In `@packages/davinci-client/src/lib/node.reducer.ts`:
- Around line 144-145: The SINGLE_CHECKBOX branch in node.reducer.ts is changing
the public contract for non-required fields by always returning
ValidatedBooleanCollector, which breaks callers that still expect
BooleanCollector for required: false cases. Update the return logic in the
reducer path for SINGLE_CHECKBOX so non-required fields keep the legacy
BooleanCollector behavior, or explicitly gate this as a breaking change by
aligning the reducer, returnValidatedBooleanCollector, and the related
tests/collector type removals together.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8929896-f0b0-43ae-b6ff-831fe09b6002
📒 Files selected for processing (16)
.changeset/curly-wolves-swim.mde2e/davinci-app/components/form-image.tse2e/davinci-app/main.tse2e/davinci-app/server-configs.tse2e/davinci-suites/src/form-image.test.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/collector.types.test-d.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.types.tspackages/davinci-client/src/lib/node.reducer.test.tspackages/davinci-client/src/lib/node.reducer.tspackages/davinci-client/src/lib/node.types.test-d.tspackages/davinci-client/src/lib/node.types.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/curly-wolves-swim.md
🚧 Files skipped from review as they are similar to previous changes (5)
- e2e/davinci-suites/src/form-image.test.ts
- e2e/davinci-app/components/form-image.ts
- packages/davinci-client/src/lib/davinci.types.ts
- e2e/davinci-app/server-configs.ts
- packages/davinci-client/src/lib/collector.utils.test.ts
Updated, thanks! client.types.ts is untouched in this PR, so skipped it! |
|
|
||
| return { | ||
| ...base, | ||
| error: null, |
There was a problem hiding this comment.
Since this is an image collector, wouldn't it make sense to set an error message when field.imageUrl is empty or not present, rather than silently returning an empty src?
There was a problem hiding this comment.
The API always returns those values but if the API changed later, it's a good fallback. You are right. At the moment, I am simply setting the error to null. Done, thanks!
There was a problem hiding this comment.
Great, thanks for updating this!
Apologies for the nitpick, but all other error messages start with capitalized words, so I think we should keep the same pattern here for consistency.
Other than that, I think it's ready to merge. I tested it locally and it worked perfectly.
SteinGabriel
left a comment
SteinGabriel
left a comment
There was a problem hiding this comment.
Other than one very small finding, this looks ready to merge. I tested it locally and it works perfectly.
Thanks, @vatsalparikh!
5 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-5101
Review Note:
The third commit davinci-client: add image collector e2e test against old env is only added temporarily. Right now, only the old Davinci env has the feature flag DV-21617-forms-image-component-sdk-response enabled. So instead of updating the form to match the old env, I've added a new file form-image.test.ts and added old Davinci config to server-configs.ts file as a separate commit. Will remove this commit as soon as the flag is enabled for new Davinci console.
Description
Adds support for the IMAGE field type in DaVinci forms. When a form includes an image component, the SDK now parses it into an ImageCollector, a read-only collector with description, imageUrl, and hyperlink properties.
How to test
Recording
Screen.Recording.2026-06-22.at.11.52.23.AM.mov
Summary by CodeRabbit
New Features
IMAGEform fields in PingOne Forms, including rendering images with optional hyperlinks.API Updates
ImageCollector/ImageField.Tests