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 39 minutes and 8 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 (18)
📝 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 0329782
💡 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 da67c92 to https://ForgeRock.github.io/ping-javascript-sdk/pr-697/da67c92fc39236205e4ae356b46eb003a87dbf6a 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 |
…ctor/AgreementCollector
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.
| * Creates a single checkbox and attaches it to the form | ||
| * @param {HTMLFormElement} formEl - The form element to attach the checkbox to | ||
| * @param {ValidatedBooleanCollector} collector - Contains the configuration | ||
| * @param {BoooleanCollector | ValidatedBooleanCollector} collector - Contains the configuration |
There was a problem hiding this comment.
Typo here:
BoooleanCollector should be BooleanCollector
| output: { | ||
| ...base.output, | ||
| content: field.content, | ||
| ...(field.type === 'AGREEMENT' && field.titleEnabled && { title: field.title ?? '' }), |
There was a problem hiding this comment.
field.title is a string. We can simplify from
field.titleEnabled && { title: field.title ?? '' }),
to
field.titleEnabled && { title: field.title }),
3 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
Refactor