feat(davinci-client): add BooleanCollector and refactor ReadOnlyCollector/AgreementCollector#697
feat(davinci-client): add BooleanCollector and refactor ReadOnlyCollector/AgreementCollector#697ancheetah wants to merge 1 commit into
Conversation
|
| Name | Type |
|---|---|
| @forgerock/davinci-client | Minor |
| @forgerock/device-client | Minor |
| @forgerock/journey-client | Minor |
| @forgerock/oidc-client | Minor |
| @forgerock/protect | Minor |
| @forgerock/sdk-types | Minor |
| @forgerock/sdk-utilities | Minor |
| @forgerock/iframe-manager | Minor |
| @forgerock/sdk-logger | Minor |
| @forgerock/sdk-oidc | Minor |
| @forgerock/sdk-request-middleware | Minor |
| @forgerock/storage | Minor |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
|
Warning Review limit reached
More reviews will be available in 52 minutes and 26 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthrough
ChangesCollector type system and e2e rendering refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 429e218
💡 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: 1
🧹 Nitpick comments (1)
packages/davinci-client/src/lib/collector.utils.ts (1)
617-625: 💤 Low valueMinor JSDoc inaccuracy: SingleCheckboxField does not have a
validationproperty.The comment mentions "validation" as one of the field properties, but
SingleCheckboxFieldonly containsrequired,errorMessage,appearance, andrichContentfor validation-adjacent concerns.📝 Suggested doc fix
/** * `@function` returnBooleanCollector - Creates a BooleanCollector (no validation). - * `@param` {SingleCheckboxField} field - The field object containing key, label, type, required, and validation. + * `@param` {SingleCheckboxField} field - The field object containing key, label, type, required, appearance, and optional richContent. * `@param` {number} idx - The index to be used in the id of the BooleanCollector. * `@returns` {BooleanCollector} The constructed BooleanCollector object. */🤖 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.utils.ts` around lines 617 - 625, The JSDoc comment for the returnBooleanCollector function incorrectly lists "validation" as a property of SingleCheckboxField. Update the `@param` description for the field parameter to remove the mention of "validation" and instead accurately list the actual properties that SingleCheckboxField contains: required, errorMessage, appearance, and richContent. This ensures the documentation matches the actual type definition.
🤖 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/main.ts`:
- Around line 292-297: The booleanComponent function call at lines 292-297
unconditionally passes davinciClient.validate(collector), but should follow the
PasswordCollector pattern shown at lines 249-251. Add a conditional check to
only call davinciClient.validate(collector) when collector.type equals
'ValidatedBooleanCollector', otherwise pass undefined. This ensures validate()
is only invoked for collectors in validated categories, preventing errors from
calling validate() on a standard BooleanCollector which is not supported by the
validation API.
---
Nitpick comments:
In `@packages/davinci-client/src/lib/collector.utils.ts`:
- Around line 617-625: The JSDoc comment for the returnBooleanCollector function
incorrectly lists "validation" as a property of SingleCheckboxField. Update the
`@param` description for the field parameter to remove the mention of "validation"
and instead accurately list the actual properties that SingleCheckboxField
contains: required, errorMessage, appearance, and richContent. This ensures the
documentation matches the actual type definition.
🪄 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: 856b6b4d-4202-41ed-a52c-0a93e2e5c47c
📒 Files selected for processing (18)
.changeset/silent-ideas-joke.md.changeset/single-checkbox.mde2e/davinci-app/components/agreement.tse2e/davinci-app/components/boolean.tse2e/davinci-app/components/label.tse2e/davinci-app/components/read-only.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.types.tspackages/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.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 with no reviewable changes (2)
- e2e/davinci-app/components/agreement.ts
- e2e/davinci-app/components/label.ts
@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✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (22.03%) 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 #697 +/- ##
==========================================
+ Coverage 18.07% 22.03% +3.95%
==========================================
Files 155 157 +2
Lines 24398 25206 +808
Branches 1203 1479 +276
==========================================
+ Hits 4410 5554 +1144
+ Misses 19988 19652 -336
🚀 New features to boost your workflow:
|
|
Deployed 5b333d8 to https://ForgeRock.github.io/ping-javascript-sdk/pr-697/5b333d823a11d70cb2ec09569111e7bc3e171199 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 - 53.8 KB (-0.0 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 |
ancheetah
force-pushed
the
SDKS-5174-update-collectors
branch
from
0329782 to
f802fd8
Compare
ancheetah
requested review from
SteinGabriel,
cerebrl,
ryanbas21 and
vatsalparikh
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
Code looks good!
Tested Form Validation page and clicked Submit by skipping I agree to the terms and conditions. The page returned success, which seems surprising. Question, is this how davinci server works? Does it ignore validation even though validation is set to true for ValidatedBooleanCollector? I want to make sure I understand this before I approve.
| output: { | ||
| ...base.output, | ||
| content: field.content, | ||
| ...(field.type === 'AGREEMENT' && field.titleEnabled && { title: field.title ?? '' }), |
There was a problem hiding this comment.
I noticed that returnReadOnlyCollector discards AgreementField.enable, which is a required field in the AgreementField type. This could have a real behavioral impact, since disabled agreements could still render. Wanted to flag it and confirm this is intentional rather than a regression.
There was a problem hiding this comment.
This is a good question. What I've observed is that if you disable agreements then DaVinci will not send the field at all. However this behavior could change in the future. @cerebrl what do you think about exposing enabled in the collector? And if we do then where should it go, on the collector output?
There was a problem hiding this comment.
Alternatively, we could check for field.enabled in the node reducer and skip producing the collector if enabled: false
There was a problem hiding this comment.
Does the enabled refer to the Agreement itself? If so, we don't really have that concept at all in collectors. A collector is either present or not present. I don't think we have the idea of present, but disabled, yeah?
Okay, then we just need to update the e2e davinci app for it to validate client side. |
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
Once we get clarity around the agreement field having enabled: false, this is good to merge.
I take it back. Each input in the validation form behaves like this. We don't actually block submission in our e2e, we just check for the error message in the test suite. In the sample app I will make sure to block submission if the box isn't checked. |
ancheetah
force-pushed
the
SDKS-5174-update-collectors
branch
from
f802fd8 to
ae4d00f
Compare
![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.
…ctor/AgreementCollector
ancheetah
force-pushed
the
SDKS-5174-update-collectors
branch
from
ae4d00f to
429e218
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 has identified a possible root cause for your failed CI:
We investigated the E2E test failure in @forgerock/davinci-suites:e2e-ci--src/form-fields.test.ts and determined it is unrelated to this PR, as @forgerock/davinci-suites was not modified by these changes. The identical timeout failure was confirmed to exist in an independent branch (684), indicating a pre-existing environment or infrastructure issue with the test setup.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
5 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-5174
Description
What
Adds a
BooleanCollectorfor non-requiredSINGLE_CHECKBOXfields, removing the need to useValidatedBooleanCollectorwhen no validation is needed. Also consolidatesAgreementCollectorintoReadOnlyCollectorby adding an optionaltitlefield, eliminating a redundant collector type.Why
SINGLE_CHECKBOXfields withoutrequired: truewere incorrectly mapped toValidatedBooleanCollector, implying validation constraints that don't exist. Separately,AgreementCollectorwas a one-off type with bespoke output shape that duplicatedReadOnlyCollector's responsibility — agreement content is read-only display text with an optional title.Changes
BooleanCollectortype —SINGLE_CHECKBOXfields withrequired: falsenow produce aBooleanCollectorinstead ofValidatedBooleanCollector; the node reducer branches onrequiredto pick the correct collectorAgreementCollectorremoved —AGREEMENTfields are now handled byreturnReadOnlyCollector, which conditionally setstitlewhentitleEnabledis true;ReadOnlyCollectorgains an optionaltitle?: stringoutput propertyreturnAgreementCollectordeleted — its logic folded intoreturnReadOnlyCollector;AGREEMENTfield type added to theLABELbranch innode.reducer.tsValidatedCollectorsunion type — extracted from the inlineValidator<T>default, now used as a proper constraint (T extends ValidatedCollectors)agreement.tsandlabel.tscomponents deleted; replaced by a unifiedread-only.tsthat handlesReadOnlyCollector(with optional title) andRichTextCollector;main.tsupdated to route bothBooleanCollectorandValidatedBooleanCollectorthrough the same boolean componentreturnBooleanCollectorunit tests, updated agreement collector tests to assertReadOnlyCollectoroutput, added a fullBooleanCollectordescribe block innode.reducer.test.ts, and fixedValidatedBooleanCollectortest fixtures to userequired: trueSummary by CodeRabbit
Release Notes
New Features
Changes / Bug Fixes
Type Updates
requiredfor single-checkbox andtitlefor agreement fields are now optional.