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
2 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