feat(davinci-client): support single checkbox component#629
Conversation
|
📝 WalkthroughWalkthroughThis PR introduces a new ChangesValidatedBooleanCollector Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 f4d8163 ☁️ 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.
| checkbox.addEventListener('change', (event) => { | ||
| const target = event.target as HTMLInputElement; | ||
| updater(target.value === 'checked'); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify checkbox event handling in similar components
# Search for other checkbox change event handlers in the codebase
rg -n -A3 -B3 "addEventListener\s*\(\s*['\"]change['\"]" --glob "*.ts" --glob "*.js" -g '!node_modules'Repository: ForgeRock/ping-javascript-sdk
Length of output: 7149
🏁 Script executed:
cat -n e2e/davinci-app/components/boolean.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 2558
Use target.checked instead of target.value === 'checked'.
The checkbox value attribute (line 39) is a static string that never changes based on checkbox state. The expression target.value === 'checked' will always evaluate to true, regardless of whether the checkbox is checked or unchecked. This breaks the boolean field functionality. Use target.checked to get the actual checkbox state, which is also the pattern used in other checkbox handlers throughout the codebase (e.g., terms-and-conditions.ts, confirmation.ts).
Proposed fix
checkbox.addEventListener('change', (event) => {
const target = event.target as HTMLInputElement;
- updater(target.value === 'checked');
+ updater(target.checked);
});📝 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.
| checkbox.addEventListener('change', (event) => { | |
| const target = event.target as HTMLInputElement; | |
| updater(target.value === 'checked'); | |
| }); | |
| checkbox.addEventListener('change', (event) => { | |
| const target = event.target as HTMLInputElement; | |
| updater(target.checked); | |
| }); |
🤖 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 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.
| const validationArray = []; | ||
|
|
||
| if ('validation' in field) { | ||
| if ('validation' in field && field.validation && 'regex' in field.validation) { |
There was a problem hiding this comment.
Critical: Validation logic regression for required-only text fields.
The condition on line 250 has been changed from:
} else if ('validation' in field || 'required' in field) {to:
} else if ('validation' in field && field.validation && 'regex' in field.validation) {This is a breaking change that affects existing functionality:
Before: Text fields with either validation OR required: true would receive validation handling and return a ValidatedTextCollector.
After: Only text fields with regex validation enter this branch. Text fields with only required: true (no regex) will skip this branch entirely and fall through to the generic SingleValueCollector at line 285, losing their validation behavior.
Impact: Required-only text fields will no longer be validated, which is a regression.
Root cause: The change appears to be an unintended side effect of adding the ValidatedBooleanCollector branch above.
🔧 Proposed fix
Restore the original OR logic to preserve validation for required-only fields:
- } else if ('validation' in field && field.validation && 'regex' in field.validation) {
+ } else if ('validation' in field || 'required' in field) {
const validationArray = [];
if ('validation' in field && field.validation && 'regex' in field.validation) {The inner conditions at lines 250-256 and 257-263 already safely check for the presence of regex and required respectively, so the outer condition should use OR.
🤖 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` at 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.
| 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.
1 participant
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4926
Description
Adds support for single checkbox component through a ValidatedBooleanCollector
Summary by CodeRabbit
Release Notes