feat(davinci-client): add form image collector#698
Conversation
🦋 Changeset detectedLatest commit: ae48a69 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ChangesImageCollector Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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 ae48a69
💡 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.
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 has identified a possible root cause for your failed CI:
We investigated the form-fields.test.ts timeout and determined it is not caused by this PR's changes. The failure occurs while waiting for a POST to an external PingOne/DaVinci customForm endpoint — a network dependency unrelated to the additive ImageCollector code introduced here. We recommend re-running the pipeline once the external service has recovered.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
@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 1bf304e to https://ForgeRock.github.io/ping-javascript-sdk/pr-698/1bf304e8f150cd33d57a82f26a8edc5e7e935fc8 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.02%) 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.02% +3.95%
==========================================
Files 155 157 +2
Lines 24398 25205 +807
Branches 1203 1479 +276
==========================================
+ Hits 4410 5552 +1142
+ Misses 19988 19653 -335
🚀 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.
| imageUrl: string; | ||
| description: string; | ||
| hyperlinkUrl?: string; |
There was a problem hiding this comment.
I'd like to avoid compound property names, if possible. How do you feel about borrowing from HTML here and just use src, instead of imageUrl and href, instead of hyperlinkUrl?
There was a problem hiding this comment.
I wanted to keep the parity with what we receive from Davinci, but totally open to using simpler names. This is what we receive from Davinci
"type": "IMAGE",
"key": "<field key>",
"description": "<alt text>",
"imageUrl": "<absolute URL>",
"hyperlinkUrl": "<absolute URL>"
Do you have a preference there? I feel it's easier to reason through and debug when we keep the same properties as what Davinci returns. Besides, I checked and almost all collectors except for QrCodeField follow property names the same as what Davinci returns.
I can change it to alt, src, href. However, would be good to know your opinion on whether we should keep that parity or borrow from HTML.
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
Lgtm, some minor comments and suggestions
| output: { | ||
| key: `${field.key || field.type}-${idx}`, | ||
| label: field.content, | ||
| label: 'content' in field ? field.content : '', |
There was a problem hiding this comment.
What do you think about copying field.description here into label so it's not empty?
There was a problem hiding this comment.
I see what you mean, something like field.content ?? field.description ?? ''
Among the NoValueCollectors, description is used only by ImageCollector. We can put that as a label, but it seems like we're adding an image collector specific property to a generic NoValueCollector.
However, if we were to extend no value collector for other fields later and if we find that description is an alternative property to content, maybe we can add it there then.
Right now, I feel like it might cause future bugs where NoValueCollector now checks for description if content is null, which is something specific to only image collector, and description may not be a property or description might mean something else for other no value collectors that we add later.
There was a problem hiding this comment.
What if we only override the label property in the returnImageCollector function?
|
@vatsalparikh One more question, should this have a |
Is there a reason to hold back ImageCollector feature in 2.1 release? |
4 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
IMAGEform fields from DaVinci Forms, enabling images to render within form fields.ImageCollector/ImageFieldandIMAGEread-only fields.