feat(davinci-client): support single checkbox component#629
Conversation
🦋 Changeset detectedLatest commit: 913b98b 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 |
📝 WalkthroughWalkthroughThis PR adds support for checkbox-based boolean input to the DaVinci client by introducing a new ChangesValidatedBooleanCollector 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 913b98b
☁️ 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: 4
🧹 Nitpick comments (3)
e2e/davinci-app/components/boolean.ts (2)
1-6: 💤 Low valueNitpick: Copyright year inconsistency.
This file uses copyright year 2026, while other files in the codebase use 2025. Consider using 2025 for consistency, unless this is intentional.
🤖 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 `@e2e/davinci-app/components/boolean.ts` around lines 1 - 6, Update the copyright header year from 2026 to 2025 in the top-of-file comment block in e2e/davinci-app/components/boolean.ts so it matches the rest of the repository; edit the existing comment lines (the block that starts with "Copyright (c) 2026 Ping Identity Corporation") to use 2025 instead.
24-28: 💤 Low valueOptional: Duplicate label might cause confusion.
The component creates a group label (lines 26-28) and an individual checkbox label (lines 41-43), both displaying the same text from
collector.output.label. For a single checkbox component, this creates visual duplication. Consider removing the group label or differentiating the text.♻️ Possible refinement
- // Create a heading/label for the checkbox group - const groupLabel = document.createElement('div'); - groupLabel.textContent = collector.output.label || 'Single Checkbox'; - groupLabel.className = 'single-checkbox-label'; - containerDiv.appendChild(groupLabel); - // Create checkboxes for each option const wrapper = document.createElement('div'); wrapper.className = 'checkbox-wrapper';🤖 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 `@e2e/davinci-app/components/boolean.ts` around lines 24 - 28, The component currently creates a duplicate visible label by appending a groupLabel (const groupLabel) with text from collector.output.label and also rendering an individual checkbox label later, causing visual duplication in the single-checkbox UI; fix by removing or hiding the redundant group label when rendering a single checkbox — either stop creating/appending groupLabel (remove the containerDiv.appendChild(groupLabel) lines) or change groupLabel.textContent to a different contextual string (or add a CSS class to visually hide it) and ensure the individual checkbox label (the per-checkbox label element) remains the primary visible label.packages/davinci-client/src/lib/collector.types.ts (1)
634-635: 💤 Low valueFormatting change unrelated to PR objective.
The reformatting of
AttestationValueandAssertionValuefrom multiline to single-lineOmitexpressions appears unrelated to the checkbox support feature. While this doesn't change functionality, consider whether this formatting change belongs in a separate PR focused on code style.Also applies to: 653-654
🤖 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 634 - 635, The PR contains unrelated formatting changes: the multiline Omit types for AttestationValue and AssertionValue were collapsed into single-line forms; revert these formatting-only edits so they remain multiline (as before) and move any styling-only changes to a separate PR. Locate the AttestationValue and AssertionValue type declarations in collector.types.ts and restore their original multiline formatting (preserving the exact type members: Omit<PublicKeyCredential, 'rawId' | 'response' | 'getClientExtensionResults' | 'toJSON'> and Omit<PublicKeyCredential, 'rawId' | 'response' | 'getClientExtensionResults' | 'toJSON'> respectively), ensuring no functional changes are introduced.
🤖 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/components/boolean.ts`:
- Around line 34-39: The SingleCheckboxField's required flag isn't applied to
the created checkbox element; update the element creation in the component that
builds the checkbox (where checkbox is created from collector.output) to set
checkbox.required = !!collector.output.required (and optionally
checkbox.setAttribute('aria-required','true') for accessibility) so the HTML
required validation is honored; ensure you reference collector.output.required
and the checkbox element in the SingleCheckboxField handling logic.
- Around line 46-49: The change handler for the checkbox uses the static value
attribute and compares target.value === 'checked', which is always true; update
the event listener on checkbox (the callback passed to
checkbox.addEventListener('change', ...)) to call updater with the actual
boolean state using target.checked instead of comparing target.value, i.e.,
replace the expression target.value === 'checked' with target.checked so the
updater receives the real checked state.
In `@packages/davinci-client/src/lib/collector.utils.ts`:
- Line 250: The new AND check causes required-only text fields to skip
validation; revert the outer conditional in the text-field branch to use OR
(i.e. change "else if ('validation' in field && field.validation && 'regex' in
field.validation)" back to "else if ('validation' in field || 'required' in
field)") so that fields with either validation or required:true are handled by
the ValidatedTextCollector/validation logic (the inner branches already
distinguish regex vs required), preventing them from falling through to
SingleValueCollector.
In `@packages/davinci-client/src/lib/node.reducer.ts`:
- Around line 196-198: The SINGLE_CHECKBOX branch in node.reducer.ts currently
calls returnValidatedBooleanCollector(field, idx) without passing the prefill
value; update the case for 'SINGLE_CHECKBOX' to extract the data variable from
formData (same way other branches do) and call
returnValidatedBooleanCollector(field, idx, data). Then update
returnValidatedBooleanCollector to accept the extra data parameter and forward
it into returnSingleValueCollector so SINGLE_CHECKBOX supports prefilled values,
mirroring how returnTextCollector and returnSingleSelectCollector handle the
data.
---
Nitpick comments:
In `@e2e/davinci-app/components/boolean.ts`:
- Around line 1-6: Update the copyright header year from 2026 to 2025 in the
top-of-file comment block in e2e/davinci-app/components/boolean.ts so it matches
the rest of the repository; edit the existing comment lines (the block that
starts with "Copyright (c) 2026 Ping Identity Corporation") to use 2025 instead.
- Around line 24-28: The component currently creates a duplicate visible label
by appending a groupLabel (const groupLabel) with text from
collector.output.label and also rendering an individual checkbox label later,
causing visual duplication in the single-checkbox UI; fix by removing or hiding
the redundant group label when rendering a single checkbox — either stop
creating/appending groupLabel (remove the containerDiv.appendChild(groupLabel)
lines) or change groupLabel.textContent to a different contextual string (or add
a CSS class to visually hide it) and ensure the individual checkbox label (the
per-checkbox label element) remains the primary visible label.
In `@packages/davinci-client/src/lib/collector.types.ts`:
- Around line 634-635: The PR contains unrelated formatting changes: the
multiline Omit types for AttestationValue and AssertionValue were collapsed into
single-line forms; revert these formatting-only edits so they remain multiline
(as before) and move any styling-only changes to a separate PR. Locate the
AttestationValue and AssertionValue type declarations in collector.types.ts and
restore their original multiline formatting (preserving the exact type members:
Omit<PublicKeyCredential, 'rawId' | 'response' | 'getClientExtensionResults' |
'toJSON'> and Omit<PublicKeyCredential, 'rawId' | 'response' |
'getClientExtensionResults' | 'toJSON'> respectively), ensuring no functional
changes are introduced.
🪄 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: 524a05e9-77c1-451f-9f1a-fa7ce5b9287b
📒 Files selected for processing (12)
.gitignoree2e/davinci-app/components/boolean.tse2e/davinci-app/main.tspackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/client.types.tspackages/davinci-client/src/lib/collector.types.tspackages/davinci-client/src/lib/collector.utils.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/lib/node.reducer.tspackages/davinci-client/src/lib/node.types.test-d.tspackages/davinci-client/src/lib/node.types.tspackages/davinci-client/tsconfig.json
💤 Files with no reviewable changes (1)
- packages/davinci-client/tsconfig.json
| const checkbox = document.createElement('input'); | ||
| checkbox.type = 'checkbox'; | ||
| checkbox.id = collector.output.key; | ||
| checkbox.name = collector.output.key || 'single-checkbox-field'; | ||
| checkbox.checked = collector.output.value as boolean; | ||
| checkbox.value = 'checked'; |
There was a problem hiding this comment.
Minor: Consider adding required attribute support.
The SingleCheckboxField type includes a required property, but the component doesn't set the HTML required attribute on the checkbox element. This means required validation won't work as expected.
🛡️ Proposed fix
const checkbox = document.createElement('input');
checkbox.type = 'checkbox';
checkbox.id = collector.output.key;
checkbox.name = collector.output.key || 'single-checkbox-field';
checkbox.checked = collector.output.value as boolean;
checkbox.value = 'checked';
+ if (collector.output.required) {
+ checkbox.required = true;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const checkbox = document.createElement('input'); | |
| checkbox.type = 'checkbox'; | |
| checkbox.id = collector.output.key; | |
| checkbox.name = collector.output.key || 'single-checkbox-field'; | |
| checkbox.checked = collector.output.value as boolean; | |
| checkbox.value = 'checked'; | |
| const checkbox = document.createElement('input'); | |
| checkbox.type = 'checkbox'; | |
| checkbox.id = collector.output.key; | |
| checkbox.name = collector.output.key || 'single-checkbox-field'; | |
| checkbox.checked = collector.output.value as boolean; | |
| checkbox.value = 'checked'; | |
| if (collector.output.required) { | |
| checkbox.required = true; | |
| } |
🤖 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 `@e2e/davinci-app/components/boolean.ts` around lines 34 - 39, The
SingleCheckboxField's required flag isn't applied to the created checkbox
element; update the element creation in the component that builds the checkbox
(where checkbox is created from collector.output) to set checkbox.required =
!!collector.output.required (and optionally
checkbox.setAttribute('aria-required','true') for accessibility) so the HTML
required validation is honored; ensure you reference collector.output.required
and the checkbox element in the SingleCheckboxField handling logic.
| case 'SINGLE_CHECKBOX': { | ||
| return returnValidatedBooleanCollector(field, idx); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the signature and implementation of returnValidatedBooleanCollector
# Find the implementation of returnValidatedBooleanCollector
ast-grep --pattern $'export function returnValidatedBooleanCollector($$$) {
$$$
}'
# Also search for its usage to understand the expected parameters
rg -n -A10 "returnValidatedBooleanCollector" --type tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 4218
🏁 Script executed:
#!/bin/bash
# Check returnSingleValueCollector implementation
rg -n -B5 -A15 "export function returnSingleValueCollector" packages/davinci-client/src/lib/collector.utils.ts
# Check returnTextCollector signature and implementation
rg -n -B5 -A15 "export function returnTextCollector" packages/davinci-client/src/lib/collector.utils.ts
# Get more context around the SINGLE_CHECKBOX case in node.reducer.ts
# to understand where data comes from
rg -n -B20 "case 'SINGLE_CHECKBOX'" packages/davinci-client/src/lib/node.reducer.ts | head -50Repository: ForgeRock/ping-javascript-sdk
Length of output: 3362
🏁 Script executed:
#!/bin/bash
# Get the full implementation of returnSingleValueCollector
sed -n '155,200p' packages/davinci-client/src/lib/collector.utils.ts
# Also check what happens after line 200 to see how data is used
sed -n '155,250p' packages/davinci-client/src/lib/collector.utils.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 4790
🏁 Script executed:
#!/bin/bash
# Get more context around SINGLE_CHECKBOX to understand the data variable
# and see how similar field types handle boolean-like data
sed -n '150,210p' packages/davinci-client/src/lib/node.reducer.ts
# Also check RADIO and DROPDOWN cases mentioned in the review
rg -n -B2 -A5 "case 'RADIO':|case 'DROPDOWN':" packages/davinci-client/src/lib/node.reducer.ts | head -40Repository: ForgeRock/ping-javascript-sdk
Length of output: 2838
🏁 Script executed:
#!/bin/bash
# Check how the data value flows through for ValidatedBooleanCollector
# See if there's any conversion from boolean to string
sed -n '210,250p' packages/davinci-client/src/lib/collector.utils.ts
# Also check if there are TypeScript type definitions for ValidatedBooleanCollector
rg -n "ValidatedBooleanCollector" packages/davinci-client/src --type ts | head -20Repository: ForgeRock/ping-javascript-sdk
Length of output: 3503
Add prefill support for SINGLE_CHECKBOX field type.
The SINGLE_CHECKBOX case doesn't extract or pass the data variable to returnValidatedBooleanCollector, unlike all other field types (e.g., TEXT, RADIO, DROPDOWN). This prevents checkboxes from supporting prefilled values from formData. Both the call site in node.reducer.ts and the returnValidatedBooleanCollector function need to be updated to pass the data through to returnSingleValueCollector, similar to how returnTextCollector and returnSingleSelectCollector handle it.
🤖 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 196 - 198, The
SINGLE_CHECKBOX branch in node.reducer.ts currently calls
returnValidatedBooleanCollector(field, idx) without passing the prefill value;
update the case for 'SINGLE_CHECKBOX' to extract the data variable from formData
(same way other branches do) and call returnValidatedBooleanCollector(field,
idx, data). Then update returnValidatedBooleanCollector to accept the extra data
parameter and forward it into returnSingleValueCollector so SINGLE_CHECKBOX
supports prefilled values, mirroring how returnTextCollector and
returnSingleSelectCollector handle the data.
ancheetah
force-pushed
the
SDKS-4926-single-checkbox
branch
from
f4d8163 to
af9e242
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/davinci-app/main.ts (1)
298-300: ⚡ Quick winConsider adding validation support for ValidatedBooleanCollector.
The
ValidatedBooleanCollectorhandler passes onlydavinciClient.update(collector)tobooleanComponent, but the collector type includes validation rules and the name indicates validation support. Compare withTextCollectorhandling (lines 234-240), which passes bothupdate()andvalidate().If the boolean component needs to enforce "required" validation (e.g., mandatory checkbox acceptance), consider passing
davinciClient.validate(collector)as a third argument.♻️ Potential enhancement
} else if (collector.type === 'ValidatedBooleanCollector') { - booleanComponent(formEl, collector, davinciClient.update(collector)); + booleanComponent( + formEl, + collector, + davinciClient.update(collector), + davinciClient.validate(collector), + ); }🤖 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 `@e2e/davinci-app/main.ts` around lines 298 - 300, The ValidatedBooleanCollector branch currently calls booleanComponent(formEl, collector, davinciClient.update(collector)) but omits validation; modify the ValidatedBooleanCollector handling so booleanComponent receives the validate handler as well (e.g., pass davinciClient.validate(collector) in addition to davinciClient.update(collector)), mirroring how TextCollector supplies both update() and validate(); update the call site at the ValidatedBooleanCollector branch to include davinciClient.validate(collector) so required/other validation rules on the collector are enforced.
🤖 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.
Nitpick comments:
In `@e2e/davinci-app/main.ts`:
- Around line 298-300: The ValidatedBooleanCollector branch currently calls
booleanComponent(formEl, collector, davinciClient.update(collector)) but omits
validation; modify the ValidatedBooleanCollector handling so booleanComponent
receives the validate handler as well (e.g., pass
davinciClient.validate(collector) in addition to
davinciClient.update(collector)), mirroring how TextCollector supplies both
update() and validate(); update the call site at the ValidatedBooleanCollector
branch to include davinciClient.validate(collector) so required/other validation
rules on the collector are enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ff5f004-90c6-4033-83d2-2d5eaa35204e
📒 Files selected for processing (15)
.gitignoree2e/davinci-app/components/boolean.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.store.tspackages/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.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/lib/node.reducer.tspackages/davinci-client/src/lib/node.types.test-d.tspackages/davinci-client/src/lib/node.types.tspackages/davinci-client/tsconfig.json
💤 Files with no reviewable changes (1)
- packages/davinci-client/tsconfig.json
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- packages/davinci-client/src/lib/node.types.ts
- packages/davinci-client/src/lib/node.types.test-d.ts
- packages/davinci-client/api-report/davinci-client.api.md
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/davinci-client/src/lib/client.types.ts
- e2e/davinci-app/components/boolean.ts
- packages/davinci-client/src/lib/client.store.ts
- packages/davinci-client/src/lib/node.reducer.ts
- packages/davinci-client/src/lib/davinci.types.ts
- packages/davinci-client/src/lib/collector.utils.ts
| { | ||
| "path": "../sdk-effects/storage" | ||
| }, | ||
| { | ||
| "path": "../sdk-utilities" | ||
| }, | ||
| { | ||
| "path": "../sdk-types" | ||
| }, | ||
| { | ||
| "path": "../sdk-effects/sdk-request-middleware" | ||
| }, | ||
| { | ||
| "path": "../sdk-effects/oidc" | ||
| }, | ||
| { | ||
| "path": "../sdk-effects/logger" | ||
| }, |
There was a problem hiding this comment.
this seems aggressive, are we sure we're in sync?
There was a problem hiding this comment.
It should be added back in the latest push. For some reason pnpm build and git commit keep switching between adding and removing these.
| ) { | ||
| if (collector.type === 'ValidatedBooleanCollector') { | ||
| if (typeof action.payload.value !== 'boolean') { | ||
| throw new Error('Value argument must be a boolean'); |
There was a problem hiding this comment.
I think we should try to avoid further tech debt here. this is impure in a reducer.
There was a problem hiding this comment.
Can we do it after we merge your pr that already addresses this? It looks like that is still in review.
There was a problem hiding this comment.
I'm okay with throwing in a reducer for this PR, but I agree with Ryan that we need to refactor this sooner than later. Let's have this discussion soon and land on a holistic solution.
There was a problem hiding this comment.
Ryan has an open PR here. Let's try to prioritize this week. #626
| // Add event listener to handle single-select behavior | ||
| checkbox.addEventListener('change', (event) => { | ||
| const target = event.target as HTMLInputElement; | ||
| updater(target.value === 'checked'); |
There was a problem hiding this comment.
I think we want target.checked here, value is always 'checked'?
There was a problem hiding this comment.
If I'm understanding this correctly, the value is only checked when the checkbox is actually checked. I can explore more when I write the e2e.
https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/input/checkbox#value_2
There was a problem hiding this comment.
Nevermind, coderabbbit is complaining about the same thing. I'll fix it now
| | AutoCollectors, | ||
| ) { | ||
| const rules = collector.input.validation; | ||
| return (value: string | string[] | Record<string, unknown>) => { |
There was a problem hiding this comment.
I think we need to add boolean to this function argument type, right?
There was a problem hiding this comment.
I've updated the entire returnValidator helper with generics so we have type narrowing on the returned validator input param. Added boolean.
| key: string; | ||
| value: string | number | boolean; | ||
| type: string; | ||
| validation: (ValidationRequired | ValidationRegex)[]; |
There was a problem hiding this comment.
What is the scenario where we use the ValidationRegex in the runtime? I don't see us using it, we just add a specific object type to the validation property in the collector builder.
There was a problem hiding this comment.
I believe it's for ValidatedTextCollector
| return returnObjectValueCollector(field, idx, prefillData); | ||
| } | ||
| case 'SINGLE_CHECKBOX': { | ||
| return returnValidatedBooleanCollector(field, idx); |
There was a problem hiding this comment.
Do we want to pass in prefillData here also? What if DaVinci says to pre-check the box?
There was a problem hiding this comment.
I don't see this option in DaVinci but I'll double check with Andy
ancheetah
force-pushed
the
SDKS-4926-single-checkbox
branch
from
af9e242 to
48f2b8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/davinci-client/src/lib/node.reducer.test.ts (1)
1433-1598: ⚡ Quick winConsider adding formData initialization test for consistency.
The test suite covers the core functionality well. However, other collector types in this file (e.g., TextCollector at lines 145-221, PhoneNumberCollector at lines 839-889) include tests for populating initial values from formData. Consider adding a similar test for
ValidatedBooleanCollectorto maintain consistency, unless pre-populating boolean checkboxes from formData is intentionally not supported.Example test structure:
it('should populate ValidatedBooleanCollector with formData value', () => { const action = { type: 'node/next', payload: { fields: [ { key: 'agree', type: 'SINGLE_CHECKBOX', inputType: 'BOOLEAN', label: 'I agree to the terms', required: false, }, ], formData: { value: { agree: true, }, }, }, }; // Assert that output.value is true });🤖 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.test.ts` around lines 1433 - 1598, Add a test that verifies nodeCollectorReducer populates a ValidatedBooleanCollector from formData by dispatching a 'node/next' action whose payload includes fields with key 'agree' (type 'SINGLE_CHECKBOX', inputType 'BOOLEAN') and formData containing value: { agree: true }, then assert the returned collector (type ValidatedBooleanCollector) has output.value === true (and input.value === true if expected); if pre-population is intentionally unsupported, instead add a test asserting that formData boolean is ignored and document that behavior in the test name. Reference nodeCollectorReducer and ValidatedBooleanCollector to locate where to add the test.
🤖 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-fields.test.ts`:
- Line 56: The test should actively interact with the single-checkbox-field and
the event handler in boolean.ts should read the checkbox state via
target.checked instead of comparing target.value to the hardcoded 'checked';
update the form-fields.test.ts to add a .check() call for
'single-checkbox-field' (matching how other checkboxes are exercised) and fix
boolean.ts by replacing the target.value === 'checked' comparison with using
target.checked so the handler reflects the real checked state.
---
Nitpick comments:
In `@packages/davinci-client/src/lib/node.reducer.test.ts`:
- Around line 1433-1598: Add a test that verifies nodeCollectorReducer populates
a ValidatedBooleanCollector from formData by dispatching a 'node/next' action
whose payload includes fields with key 'agree' (type 'SINGLE_CHECKBOX',
inputType 'BOOLEAN') and formData containing value: { agree: true }, then assert
the returned collector (type ValidatedBooleanCollector) has output.value ===
true (and input.value === true if expected); if pre-population is intentionally
unsupported, instead add a test asserting that formData boolean is ignored and
document that behavior in the test name. Reference nodeCollectorReducer and
ValidatedBooleanCollector to locate where to add the test.
🪄 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: 1217b03b-5475-4f7d-ba59-9b4d0175f8af
📒 Files selected for processing (20)
.changeset/single-checkbox.md.gitignoree2e/davinci-app/components/boolean.tse2e/davinci-app/main.tse2e/davinci-suites/src/form-fields.test.tspackage.jsonpackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/client.types.test-d.tspackages/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/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 (4)
- packages/davinci-client/src/lib/node.types.test-d.ts
- .changeset/single-checkbox.md
- packages/davinci-client/src/lib/collector.utils.test.ts
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/davinci-client/src/lib/client.store.ts
- e2e/davinci-app/main.ts
- packages/davinci-client/src/lib/node.types.ts
- packages/davinci-client/src/lib/collector.utils.ts
- packages/davinci-client/src/lib/node.reducer.ts
- packages/davinci-client/src/lib/collector.types.ts
ancheetah
force-pushed
the
SDKS-4926-single-checkbox
branch
from
48f2b8e to
913b98b
Compare
@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❌ Patch coverage is
❌ Your project status has failed because the head coverage (18.21%) 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 #629 +/- ##
==========================================
+ Coverage 18.07% 18.21% +0.13%
==========================================
Files 155 155
Lines 24398 24440 +42
Branches 1203 1212 +9
==========================================
+ Hits 4410 4451 +41
- Misses 19988 19989 +1
🚀 New features to boost your workflow:
|
|
Deployed a74194e to https://ForgeRock.github.io/ping-javascript-sdk/pr-629/a74194e123b0672eabf3aa38304687ae2e23ba62 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.1 KB (+0.7 KB) ➖ No Changes➖ @forgerock/storage - 1.5 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/davinci-client/src/lib/collector.utils.test.ts (1)
1679-1680: ⚡ Quick winImport
assertexplicitly fromvitestto avoid globals coupling.Lines 1679, 1712, 1727, 1745, and 1780 use
assert()without importing it, relying on Vitest'sglobals: trueconfiguration. Making this explicit keeps tests stable if the global configuration changes.Suggested patch
-import { describe, it, expect } from 'vitest'; +import { describe, it, expect, assert } from 'vitest';🤖 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.test.ts` around lines 1679 - 1680, Tests currently call assert() as a global; explicitly import it from Vitest to avoid coupling to globals. Add "import { assert } from 'vitest';" at the top of the test file (so references to assert() in the file—e.g., the assertions around errors at the shown diff and the other occurrences) use the imported symbol instead of the global, and run/adjust any lints if needed.packages/davinci-client/src/lib/password-policy.rules.ts (1)
11-11: ⚡ Quick winChange
Validatorto a type-only import to match the file's type import convention.
Validatoris used only as a type annotation (lines 92, 94) and should be imported asimport typefor consistency with other type imports in the file (lines 9–10) and to clarify intent.Suggested patch
-import { Validator } from './client.types.js'; +import type { Validator } from './client.types.js';🤖 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/password-policy.rules.ts` at line 11, The import for Validator should be type-only: change the existing import statement that brings in Validator to an `import type` form so it is a type-only import consistent with other imports; update the import that currently reads `import { Validator } from './client.types.js'` to use type-only import and leave all usages (the type annotations referencing Validator in functions/variables around where it's used) unchanged.
🤖 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.
Nitpick comments:
In `@packages/davinci-client/src/lib/collector.utils.test.ts`:
- Around line 1679-1680: Tests currently call assert() as a global; explicitly
import it from Vitest to avoid coupling to globals. Add "import { assert } from
'vitest';" at the top of the test file (so references to assert() in the
file—e.g., the assertions around errors at the shown diff and the other
occurrences) use the imported symbol instead of the global, and run/adjust any
lints if needed.
In `@packages/davinci-client/src/lib/password-policy.rules.ts`:
- Line 11: The import for Validator should be type-only: change the existing
import statement that brings in Validator to an `import type` form so it is a
type-only import consistent with other imports; update the import that currently
reads `import { Validator } from './client.types.js'` to use type-only import
and leave all usages (the type annotations referencing Validator in
functions/variables around where it's used) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce203f2d-1d60-47a8-b32d-66cabde6165a
📒 Files selected for processing (21)
.changeset/single-checkbox.md.gitignoree2e/davinci-app/components/boolean.tse2e/davinci-app/main.tse2e/davinci-suites/src/form-fields.test.tspackage.jsonpackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/client.types.test-d.tspackages/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/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.tspackages/davinci-client/src/lib/password-policy.rules.ts
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- packages/davinci-client/src/lib/node.types.test-d.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- .changeset/single-checkbox.md
- packages/davinci-client/src/lib/client.store.ts
- e2e/davinci-app/main.ts
- e2e/davinci-suites/src/form-fields.test.ts
- packages/davinci-client/src/lib/client.types.test-d.ts
- packages/davinci-client/src/lib/collector.types.test-d.ts
- packages/davinci-client/src/lib/collector.utils.ts
- e2e/davinci-app/components/boolean.ts
- packages/davinci-client/src/lib/client.types.ts
- packages/davinci-client/src/lib/node.reducer.ts
- packages/davinci-client/src/lib/davinci.types.ts
- package.json
|
|
||
| /** | ||
| * A function type that polls for the current status during challenge or continue polling. | ||
| * | ||
| * @returns A promise resolving to a `PollingStatus` string indicating the current state, | ||
| * or an `InternalErrorResponse` if the poll request fails. | ||
| */ |
There was a problem hiding this comment.
It seems like this comment is referencing something that's not in this file. Was this an accident?
There was a problem hiding this comment.
It just adds a comment for the Poller interface below
| ) { | ||
| if (collector.type === 'ValidatedBooleanCollector') { | ||
| if (typeof action.payload.value !== 'boolean') { | ||
| throw new Error('Value argument must be a boolean'); |
There was a problem hiding this comment.
I'm okay with throwing in a reducer for this PR, but I agree with Ryan that we need to refactor this sooner than later. Let's have this discussion soon and land on a holistic solution.
4 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4926
Description
What
Adds a new
ValidatedBooleanCollectortype todavinci-clientto handle DaVinci flows that include a single checkbox field (SINGLE_CHECKBOX/inputType: BOOLEAN). The collector holds abooleanvalue and follows the same validated collector pattern asValidatedTextCollector. An e2e reference implementation (booleanComponent) is included in the test app.TODO: E2E test suite will be added in another ticket with agreements e2e https://pingidentity.atlassian.net/browse/SDKS-5000
Changes
New types
ValidatedBooleanCollector—ValidatedSingleValueCollectorWithValue<'ValidatedBooleanCollector', boolean>SingleCheckboxField— raw DaVinci field shape (type: 'SINGLE_CHECKBOX',inputType: 'BOOLEAN')Type improvements
SingleValueCollectorWithValue<T, V>andValidatedSingleValueCollectorWithValue<T, V>are now generic over their value type (V, defaults tostring), replacing the loosestring | number | booleanunionCollectorValueType<T>now mapsValidatedBooleanCollector→booleanSingleValueCollectorsunion updated to use explicit named aliases instead of anonymous genericsSingleValueCollectorTypesandSingleValueFieldsexpanded with'ValidatedBooleanCollector'/SingleCheckboxFieldupdateCollectorValuesaction payload now acceptsbooleanLogic
returnValidatedBooleanCollectorfactory function added tocollector.utils.tsnodeCollectorReducerhandles theSINGLE_CHECKBOXcase and enforces abooleanguard on value updatesCollectorsunion innode.types.tsincludesValidatedBooleanCollectorE2E
e2e/davinci-app/components/boolean.ts— renders a single checkbox and wiresupdaterto thechangeevente2e/davinci-app/main.tsroutesValidatedBooleanCollectorto the new componentHousekeeping
.gitignore— ignores.claude/skillsand.claude/CLAUDE.mdSummary by CodeRabbit
New Features
Tests
Documentation
Chores