[7418] FE2 -CheckboxGroup control #4653
Conversation
|
Warning Rate limit exceeded@jvega190 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 2 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a responsive, readonly-aware CheckboxGroup control with Select All, search/filtering, multi-column and virtualized rendering; wires a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CG as CheckboxGroup
participant HW as useWindowWidth
participant V as Validator
participant P as Parser
Note over HW,CG: component mount
HW->>CG: provide current width
CG->>CG: compute numColumns & determine readonly
U->>CG: type in search
CG->>CG: filter finalOptions (label & key)
U->>CG: click Select All
CG->>CG: compute selections (respect readonly)
CG-->>U: update UI (checked/indeterminate)
U->>CG: toggle option
CG->>CG: update selection state
CG->>V: run checkboxGroupValidator
V-->>CG: validity + messages
Note over P: build-time parsing
P->>P: parse legacy `minSize` -> field.validations.minSize
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/src/components/FormsEngine/lib/validators.ts (1)
23-33: FixvalidatorsMaptype and message handling
- Change from
Record<BuiltInControlType, ElementType>toRecord<BuiltInControlType, ValidatorFn | null>by addingtype ValidatorFn = (field: ContentTypeField, currentValue: unknown, messages: string[]) => boolean; export const validatorsMap: Record<BuiltInControlType, ValidatorFn | null> = { … };- Remove the undefined
defineMessagecall and push a plain string, e.g.:if (!isValid) messages.push(`Select at least ${minSelected} option${minSelected === 1 ? '' : 's'}.`);
🧹 Nitpick comments (2)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (2)
48-52: Remove hardcoded width on option label to avoid layout conflictsThe Grid defines the layout; forcing 50% width on FormControlLabel causes misalignment. Use full width or omit.
- sx={{ width: '50%' }} + sx={{ width: '100%' }}
240-264: Legacy flex-basis styling conflicts with Grid layoutThis block assumes 3 columns and can fight Grid sizing. Consider removing
flexBasisand height overrides when Grid is used.Would you like a follow-up patch to conditionally apply this only when not using the Grid container?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx(8 hunks)ui/app/src/components/FormsEngine/lib/validators.ts(1 hunks)ui/app/src/hooks/useWindowWidth.ts(1 hunks)ui/app/src/models/ContentType.ts(1 hunks)ui/app/src/services/contentTypes.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (7)
ui/app/src/models/LookupTable.ts (1)
LookupTable(17-17)ui/app/src/components/FormsEngine/controls/Checkbox.tsx (1)
Checkbox(26-35)ui/app/src/hooks/useWindowWidth.ts (1)
useWindowWidth(19-29)ui/app/src/hooks/useDebouncedInput.ts (1)
useDebouncedInput(22-32)ui/app/src/components/FormsEngine/dataSourceHooks/useKVPLoader.ts (1)
useKVPLoader(30-77)ui/app/src/components/FormsEngine/components/FormsEngineField.tsx (1)
FormsEngineField(91-279)ui/app/src/components/SearchBar/SearchBar.tsx (1)
SearchBar(61-209)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Travis CI - Pull Request
🔇 Additional comments (3)
ui/app/src/models/ContentType.ts (1)
54-56: LGTM: Validation key extensionAdding 'minSize' to ValidationKeys aligns with new constraint parsing and validators.
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (2)
29-29: No import change needed for useDebouncedInput
ui/app/src/hooks/useDebouncedInput.tsexports the hook both as a named and default export; the existing default import is valid.Likely an incorrect or invalid review comment.
80-88: Incorrect MUI Grid props: use item + xs/sm/md/lg instead of size
@mui/material/Griddoesn’t supportsize={{ ... }}. Useitemplus breakpoint props. Apply to both virtualized and non‑virtualized rows.- <Grid size={{ sm: 12, md: 6, lg: 4 }} key={option.key}> + <Grid item xs={12} md={6} lg={4} key={option.key}>Apply the same change in the non‑virtualized map.
Also applies to: 279-289
⛔ Skipped due to learnings
Learnt from: jvega190 PR: craftercms/studio-ui#4580 File: ui/app/src/components/SitesGrid/SitesGrid.tsx:58-58 Timestamp: 2025-07-18T20:43:52.177Z Learning: In MUI 7, the `size` prop is not required for Grid components. When migrating from MUI 5 to MUI 7, removing the `item` prop from Grid components without adding a `size` prop is valid and acceptable. The `size` prop is only needed when you want to explicitly control the width of the grid item.Learnt from: jvega190 PR: craftercms/studio-ui#4580 File: ui/app/src/components/SitesGrid/SitesGrid.tsx:58-58 Timestamp: 2025-07-18T20:43:52.177Z Learning: In MUI v7, the `size` prop on Grid components is optional, not required. Grid items can exist without explicit sizing and will use their natural dimensions. When migrating from MUI v5 to v7, simply removing the `item` prop without adding a `size` prop is a valid approach when you want the Grid items to size naturally based on their content.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/src/components/FormsEngine/lib/validators.ts (1)
28-63: Type error: validatorsMap assigns nulls but type excludes nullvalidatorsMap is typed as Partial<..., ValidatorFunctionDef>, so entries must be functions or undefined. Many entries are set to null, which breaks type-checking.
Simplify by only declaring implemented validators (others can be omitted), or use undefined. Recommended:
-export const validatorsMap: Partial<Record<BuiltInControlType, ValidatorFunctionDef>> = { - repeat: null, - 'auto-filename': null, - 'aws-file-upload': null, - 'checkbox-group': (field, currentValue, messages) => { /* ... */ }, - checkbox: null, - 'date-time': null, - disabled: null, - dropdown: null, - 'file-name': null, - forcehttps: null, - 'image-picker': null, - input: null, - 'internal-name': null, - label: null, - 'link-input': null, - 'link-textarea': null, - 'linked-dropdown': null, - 'locale-selector': null, - 'node-selector': null, - 'numeric-input': null, - 'page-nav-order': null, - rte: null, - textarea: null, - time: null, - 'transcoded-video-picker': null, - uuid: null, - 'video-picker': null, - colorPicker: undefined -}; +export const validatorsMap: Partial<Record<BuiltInControlType, ValidatorFunctionDef>> = { + 'checkbox-group': (field, currentValue, messages) => { + const minSelected = Number(field.validations?.minSize?.value ?? 0); + const selectedCount = Array.isArray(currentValue) ? currentValue.length : 0; + const isValid = selectedCount >= minSelected; + if (!isValid) { + messages.push(defineMessage({ defaultMessage: 'Please select at least the minimum required items.' })); + } + return isValid; + } +};
🧹 Nitpick comments (3)
ui/app/src/components/FormsEngine/components/FormsEngineField.tsx (1)
275-279: Avoid rendering “0” when messages array is emptyUsing length in a JSX logical chain can render 0. Guard with a boolean or an explicit > 0 check.
Apply this diff:
- {!isValid && - validityData?.messages?.length && - validityData.messages.map((message, key) => ( + {!isValid && + (validityData?.messages?.length ?? 0) > 0 && + validityData!.messages.map((message, key) => ( <FormHelperText key={key}>{translateIfMessageDescriptor(message, formatMessage)}</FormHelperText> ))}ui/app/src/components/FormsEngine/lib/formUtils.tsx (1)
725-731: Helper looks good; adjust IntlShape import to public APItranslateIfMessageDescriptor is correct and typed well. Prefer importing IntlShape from 'react-intl' rather than 'react-intl/src/types' to avoid relying on internal paths (important in react-intl 7.x).
Apply this change to the import near Line 38:
-import { IntlShape } from 'react-intl/src/types'; +import type { IntlShape } from 'react-intl';Based on learnings
ui/app/src/components/FormsEngine/lib/validators.ts (1)
23-28: Consider exporting ValidatorFunctionDef or inlining to the exported typeIf consumers need to type validatorsMap or external validators, export ValidatorFunctionDef, or inline its shape in the validatorsMap type to avoid non-exported types in public declarations.
Apply this diff:
-type ValidatorFunctionDef = ( +export type ValidatorFunctionDef = ( field: ContentTypeField, currentValue: unknown, messages: FieldValidityState['messages'] ) => boolean;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ui/app/src/components/FormsEngine/components/FormsEngineField.tsx(2 hunks)ui/app/src/components/FormsEngine/lib/formUtils.tsx(2 hunks)ui/app/src/components/FormsEngine/lib/validators.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
ui/app/src/components/FormsEngine/components/FormsEngineField.tsx (2)
ui/app/src/components/FormsEngine/lib/formUtils.tsx (1)
translateIfMessageDescriptor(725-730)static-assets/components/cstudio-forms/forms-engine.js (1)
formatMessage(786-786)
ui/app/src/components/FormsEngine/lib/formUtils.tsx (2)
static-assets/components/cstudio-common/common-api.js (3)
formatMessage(712-712)formatMessage(847-847)formatMessage(2333-2333)static-assets/components/cstudio-forms/forms-engine.js (1)
formatMessage(786-786)
ui/app/src/components/FormsEngine/lib/validators.ts (3)
ui/app/src/models/ContentType.ts (1)
ContentTypeField(65-143)ui/app/src/components/FormsEngine/lib/controlMap.ts (1)
BuiltInControlType(22-50)ui/app/src/utils/string.ts (1)
isEmpty(283-286)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Travis CI - Pull Request
🔇 Additional comments (1)
ui/app/src/components/FormsEngine/components/FormsEngineField.tsx (1)
46-46: Good addition: translating validation messagesImporting translateIfMessageDescriptor and using formatMessage aligns the rendering path with MessageDescriptor-based validators.
|
Outside diff range comment addressed in here |
…o feature/7418-checkbox-group # Conflicts: # ui/app/src/components/FormsEngine/components/FormsEngineField.tsx # ui/app/src/components/FormsEngine/lib/validators.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (1)
156-162: Select All logic still doesn't handle filtered views correctly.This issue was raised in the previous review but has not been fully addressed. The current implementation compares
value.length(all selected items) withfinalOptions.length(visible items), which can lead to incorrect behavior:
- When "Select All" is clicked in a filtered view, it clears ALL selections (including items not visible in the filter), rather than toggling only the visible items.
- The checked/indeterminate state at lines 186-187 will be incorrect when selections exist outside the filtered view.
Apply this diff to implement the correct logic as suggested in the previous review:
+ const visibleKeySet = useMemo(() => new Set(finalOptions.map((o) => o.key)), [finalOptions]); + const selectedInViewCount = useMemo( + () => value.reduce((acc, item) => acc + (visibleKeySet.has(item.key) ? 1 : 0), 0), + [value, visibleKeySet] + ); + const allInViewSelected = finalOptions.length > 0 && selectedInViewCount === finalOptions.length; + const someInViewSelected = selectedInViewCount > 0 && !allInViewSelected; + const checkAll = () => { - if (value.length === finalOptions.length) { - setValue([]); + if (allInViewSelected) { + // Unselect only visible ones + setValue(value.filter((item) => !visibleKeySet.has(item.key))); } else { - setValue(finalOptions.map((option) => ({ key: option.key, value_smv: option.value }))); + // Select all visible (merge with existing) + const existing = new Set(value.map((v) => v.key)); + const additions = finalOptions + .filter((o) => !existing.has(o.key) && o.value) + .map((o) => ({ key: o.key, value_smv: o.value })); + setValue(value.concat(additions)); } };Also update the checkbox state at lines 186-187:
- indeterminate={value.length > 0 && value.length < finalOptions.length} - checked={value.length === finalOptions.length} + indeterminate={someInViewSelected} + checked={allInViewSelected}
🧹 Nitpick comments (3)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (3)
41-41: Remove unusedhtmlIdvariable.The
htmlIdis declared but never used in the component. This creates unnecessary overhead and clutters the code.Apply this diff to remove the unused variable:
- const htmlId = useId(); const theme = useTheme();
47-47: Align breakpoints with MUI Grid responsive system.The
numColumnsbreakpoints (1200, 900) don't fully align with the MUI Grid responsive sizing used later (size={{ sm: 12, md: 6, lg: 4 }}), which follows MUI's standard breakpoints (sm: 600, md: 900, lg: 1200). This mismatch can cause inconsistent column counts between the responsive calculation and the actual grid layout, especially in the 600-900px range.Apply this diff to align with MUI breakpoints:
- const numColumns = windowWidth >= 1200 ? 3 : windowWidth >= 900 ? 2 : 1; + const numColumns = windowWidth >= 1200 ? 3 : windowWidth >= 900 ? 2 : windowWidth >= 600 ? 1 : 1;Or simplify to:
- const numColumns = windowWidth >= 1200 ? 3 : windowWidth >= 900 ? 2 : 1; + const numColumns = windowWidth >= 1200 ? 3 : windowWidth >= 600 ? 2 : 1;This ensures consistent behavior across all viewport sizes and matches the Grid's responsive breakpoints.
251-271: Remove redundant inline width in buildOption.Line 259 sets
width: '50%'on the FormControlLabel, but this component is always rendered inside a Grid system (both in the non-virtualized path at line 235 and the virtualized path at line 291) where the Grid's responsivesizeprop controls the width. The inline width style is redundant and could potentially conflict with the Grid's layout calculations.Apply this diff to remove the redundant width:
const buildOption = ( option: KVPLoaderItem['items'][0], onChange: CheckboxProps['onChange'], checkedValuesLookup: LookupTable<boolean>, readonly: boolean = false ) => ( <FormControlLabel key={option.key} - sx={{ width: '50%' }} control={ <Checkbox disabled={readonly} color="info" checked={checkedValuesLookup[option.key] ?? false} onChange={onChange} value={option.key} /> } label={option.value} /> );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx(6 hunks)ui/app/src/components/FormsEngine/lib/validators.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/app/src/components/FormsEngine/lib/validators.ts
🧰 Additional context used
🧬 Code graph analysis (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (7)
ui/app/src/components/FormsEngine/types.ts (1)
ControlProps(23-30)ui/app/src/hooks/useWindowWidth.ts (1)
useWindowWidth(19-32)ui/app/src/components/FormsEngine/dataSourceHooks/useKVPLoader.ts (2)
useKVPLoader(30-77)KVPLoaderItem(24-28)ui/app/src/components/FormsEngine/components/FormsEngineField.tsx (1)
FormsEngineField(92-282)ui/app/src/components/SearchBar/SearchBar.tsx (1)
SearchBar(61-209)ui/app/src/components/FormsEngine/controls/Checkbox.tsx (2)
Checkbox(26-35)CheckboxProps(22-24)ui/app/src/models/LookupTable.ts (1)
LookupTable(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (7)
52-65: LGTM! Robust JSON parsing implemented.The listDirection parsing now correctly handles malformed JSON and non-array values with try/catch and Array.isArray checks, as suggested in the previous review. This prevents runtime errors from calling
.findon objects.
129-132: LGTM! Safe filtering for placeholder items.The filtering logic now correctly guards against undefined values by checking
typeof option.value === 'string'before callingtoLowerCase(), as suggested in the previous review. This prevents runtime errors when placeholder items are present.
167-178: LGTM! Readonly state correctly propagated to SearchBar.The
readonlyprop is correctly passed asdisabledto the SearchBar component, which aligns with the component's API and ensures consistent behavior when the control is in readonly mode.
233-244: LGTM! Correct Grid responsive sizing.The Grid implementation correctly uses MUI's responsive sizing (
size={{ sm: 12, md: 6, lg: 4 }}) to display 1, 2, or 3 columns based on viewport width, and properly handles placeholder items with empty fragments to maintain the grid structure.
273-302: LGTM! VirtualRow correctly implements multi-column virtualization.The VirtualRow component correctly implements multi-column virtualization by calculating the adjusted index based on
numColumnsand slicing the appropriate range of options. It maintains consistency with the non-virtualized rendering path by using the same Grid structure and properly handles readonly state and placeholder items.
221-231: LGTM! Virtualized rendering correctly configured.The FixedSizeList is correctly configured with all necessary props including the new
numColumnsandreadonlyin the itemData, which are properly consumed by the VirtualRow component.
32-34: Dependency versions are up-to-date and secure.nanoid@^5.1.5 (≥5.0.9),@mui/material@^7.3.2, andreact-intl@^7.1.11 have no active vulnerabilities or advisories.
…rendering of 3 columns logic
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (1)
124-130: Select All logic doesn't handle filtered options correctly.The
checkAllfunction operates onfinalOptions(filtered items), but the indeterminate and checked state calculations (lines 154-155) comparevalue.lengthdirectly againstfinalOptions.length. This fails whenvaluecontains items that have been filtered out offinalOptions.Example: User selects items A, B, C, then searches to show only items D, E. The checkbox will incorrectly show as unchecked because
value.length (3) !== finalOptions.length (2), even though items might be selected.The correct approach is to count how many items in
valueare actually present in the currentfinalOptions(visible items) and use that count for the indeterminate/checked calculations.This is the same issue flagged in the past review. Please refer to the previous suggestion for the fix.
🧹 Nitpick comments (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (1)
79-92: UsenumRowsconsistently in reordering logic.Line 88 recalculates
Math.ceil(finalOptions.length / 2), which is identical to thenumRowsvariable computed on line 83. For clarity and maintainability, usenumRowsconsistently.Apply this diff:
for (let i = 0; i < numRows; i++) { sortedOptions.push(finalOptions[i]); if (finalOptions[i + numRows]) { - sortedOptions.push(finalOptions[i + Math.ceil(finalOptions.length / 2)]); + sortedOptions.push(finalOptions[i + numRows]); } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (7)
ui/app/src/components/FormsEngine/types.ts (1)
ControlProps(23-30)ui/app/src/hooks/useWindowWidth.ts (1)
useWindowWidth(19-32)ui/app/src/components/FormsEngine/dataSourceHooks/useKVPLoader.ts (2)
useKVPLoader(30-77)KVPLoaderItem(24-28)ui/app/src/components/FormsEngine/components/FormsEngineField.tsx (1)
FormsEngineField(92-282)ui/app/src/components/SearchBar/SearchBar.tsx (1)
SearchBar(61-209)ui/app/src/components/FormsEngine/controls/Checkbox.tsx (2)
Checkbox(26-35)CheckboxProps(22-24)ui/app/src/models/LookupTable.ts (1)
LookupTable(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Travis CI - Pull Request
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (5)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (5)
17-46: LGTM!The imports are appropriate, and the responsive
numColumnslogic using a 900px threshold aligns with MUI's defaultmdbreakpoint. The readonly state correctly combines form-level and field-level properties.
102-122: LGTM!The
checkedValuesLookupmemo andhandleChangelogic are correct. Line 118 appropriately looks up values from the originaloptionsarray rather thanfinalOptions, ensuring the full value is captured even when filtering is active.
189-208: LGTM!Both rendering paths (virtualized and non-virtualized) correctly use the responsive Grid layout and pass the
readonlystate through. The Grid sizing withsize={{ sm: 12, md: 6 }}aligns with thenumColumnscalculation, ensuring consistent behavior.
214-233: LGTM!The
buildOptionhelper is clean and correctly applies thereadonlystate to the Checkbox'sdisabledprop.
235-259: LGTM!The
VirtualRowcomponent correctly implements multi-column virtualization by slicing the options array based onnumColumnsand rendering them in a Grid layout consistent with the non-virtualized path.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (2)
135-141: Fix Select All to act only on visible options.The current
checkAllimplementation clears all selections when all visible items are selected, which incorrectly removes selections for filtered-out items. It should only toggle visible options while preserving other selections.Apply this diff to fix the logic:
+ const visibleKeySet = useMemo(() => new Set(finalOptions.map((o) => o.key)), [finalOptions]); + const selectedInViewCount = useMemo( + () => value.reduce((acc, item) => acc + (visibleKeySet.has(item.key) ? 1 : 0), 0), + [value, visibleKeySet] + ); + const allInViewSelected = finalOptions.length > 0 && selectedInViewCount === finalOptions.length; + const someInViewSelected = selectedInViewCount > 0 && !allInViewSelected; + const checkAll = () => { - if (value.length === finalOptions.length) { - setValue([]); + if (allInViewSelected) { + // Deselect only visible items + setValue(value.filter((item) => !visibleKeySet.has(item.key))); } else { - setValue(finalOptions.map((option) => ({ key: option.key, value_smv: option.value }))); + // Select all visible items (merge with existing) + const existing = new Set(value.map((v) => v.key)); + const additions = finalOptions + .filter((o) => !existing.has(o.key)) + .map((o) => ({ key: o.key, value_smv: o.value })); + setValue(value.concat(additions)); } };
165-166: Update Select All checkbox state to reflect visible items.The indeterminate and checked states incorrectly compare total selections against visible options. After applying the fix for the
checkAllfunction, update these props to use the computedallInViewSelectedandsomeInViewSelectedvalues.Apply this diff:
<Checkbox disabled={readonly} color="info" - indeterminate={value.length > 0 && value.length < finalOptions.length} - checked={value.length === finalOptions.length} + indeterminate={someInViewSelected} + checked={allInViewSelected} onChange={checkAll} />
🧹 Nitpick comments (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (1)
17-17: Remove unused import.
useIdis imported but never used in the component.Apply this diff:
-import React, { ChangeEvent, useId, useMemo, useState } from 'react'; +import React, { ChangeEvent, useMemo, useState } from 'react';
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (6)
ui/app/src/components/FormsEngine/types.ts (1)
ControlProps(23-30)ui/app/src/hooks/useWindowWidth.ts (1)
useWindowWidth(19-32)ui/app/src/components/FormsEngine/lib/formUtils.tsx (2)
isFieldReadOnly(764-766)getPropertyValue(749-755)ui/app/src/components/FormsEngine/dataSourceHooks/useKVPLoader.ts (2)
useKVPLoader(30-77)KVPLoaderItem(24-28)ui/app/src/components/SearchBar/SearchBar.tsx (1)
SearchBar(61-209)ui/app/src/models/LookupTable.ts (1)
LookupTable(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (7)
52-65: LGTM! Safe JSON parsing implemented.The listDirection parsing now properly handles errors and validates the parsed result is an array before using it. This addresses the previous review concern.
78-102: LGTM! Reordering and filtering logic are correct.The two-column vertical reordering correctly interleaves the first and second halves of the options array for top-down display. The filtering also safely handles undefined values with proper type checking. Both previously flagged concerns have been addressed.
110-121: LGTM! Loading skeleton provides good UX.The loading state uses consistent Grid layout and clearly indicates the component is loading with skeleton placeholders.
50-50: LGTM! Readonly propagation is correctly implemented.The
readonlystate is properly computed from both field-level and form-level settings, then consistently applied to all interactive elements (SearchBar, Select All checkbox, and individual checkboxes).Also applies to: 147-147, 163-163, 209-209, 215-215, 235-235
212-218: LGTM! Grid-based layout is well-structured.The non-virtualized rendering uses Material-UI Grid for responsive two-column layout, correctly delegating option rendering to the
buildOptionhelper.
225-244: LGTM! Clean helper function for option rendering.The
buildOptionhelper properly encapsulates checkbox rendering with readonly support and checked state lookup.
246-270: LGTM! VirtualRow correctly implements multi-column virtualization.The virtualized row component properly calculates item indices for multi-column layout and correctly passes all necessary props (including readonly) to the option renderer.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (1)
135-141: Critical: Select All logic still doesn't handle filtering correctly.This issue was flagged in a previous review but remains unaddressed. When
keywordfiltering is active,finalOptionsis a filtered subset, but the current logic:
- Compares
value.length === finalOptions.length(total selections vs. visible items)- Clears all selections when "unchecking all" (line 137)—should only clear visible selections
- Replaces value with only visible items (lines 139-140)—loses selections outside the current filter
Expected behavior:
- "Select All" should select all visible items and merge with existing selections
- "Unselect All" should unselect only visible items, preserving others
Apply the fix from the previous review:
+ const visibleKeySet = useMemo(() => new Set(finalOptions.map((o) => o.key)), [finalOptions]); + const selectedInViewCount = useMemo( + () => value.reduce((acc, item) => acc + (visibleKeySet.has(item.key) ? 1 : 0), 0), + [value, visibleKeySet] + ); + const allInViewSelected = finalOptions.length > 0 && selectedInViewCount === finalOptions.length; + const someInViewSelected = selectedInViewCount > 0 && !allInViewSelected; + const checkAll = () => { - if (value.length === finalOptions.length) { - setValue([]); + if (allInViewSelected) { + // Unselect only visible ones + setValue(value.filter((item) => !visibleKeySet.has(item.key))); } else { - setValue(finalOptions.map((option) => ({ key: option.key, value_smv: option.value }))); + // Select all visible (merge with existing) + const existing = new Set(value.map((v) => v.key)); + const additions = finalOptions + .filter((o) => !existing.has(o.key) && o.value) + .map((o) => ({ key: o.key, value_smv: o.value })); + setValue(value.concat(additions)); } };
🧹 Nitpick comments (2)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (2)
109-121: Optional: Skeleton count could respect numColumns.The loading skeleton hardcodes 2 items (line 113), but
numColumnscan be 1 on narrow screens. ConsiderArray.from({ length: numColumns })for consistency. This is purely cosmetic and doesn't affect functionality.
185-187: Optional: Remove obsoleteflexBasisstyling.Line 187's
flexBasis: calc(32.5% - ...)suggests a 3-column layout, but the component now uses Grid-based 2-column layout (lines 212-218, 262-268). The flexBasis is overridden by Grid and should be removed to avoid confusion.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (6)
ui/app/src/components/FormsEngine/types.ts (1)
ControlProps(23-30)ui/app/src/hooks/useWindowWidth.ts (1)
useWindowWidth(19-32)ui/app/src/components/FormsEngine/lib/formUtils.tsx (2)
isFieldReadOnly(764-766)getPropertyValue(749-755)ui/app/src/components/FormsEngine/dataSourceHooks/useKVPLoader.ts (2)
useKVPLoader(30-77)KVPLoaderItem(24-28)ui/app/src/components/SearchBar/SearchBar.tsx (1)
SearchBar(61-209)ui/app/src/models/LookupTable.ts (1)
LookupTable(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (4)
49-66: LGTM—Field properties are now safely extracted.All past concerns have been addressed:
selectAllproperty key is correct (line 51)listDirectionparsing is properly guarded with try-catch andArray.isArraycheck (lines 55-60)
77-102: LGTM—Options processing is correct.The 2-column vertical reordering logic (lines 80-92) correctly interleaves items top-down, and the keyword filtering (lines 98-101) safely handles missing or non-string values with type checking and optional chaining. Past concerns have been addressed.
225-244: LGTM—Clean helper function.The
buildOptionhelper is well-structured and correctly propagates thereadonlystate to disable checkboxes. The fallback?? false(line 237) safely handles missing keys.
246-270: LGTM—VirtualRow correctly implements multi-column virtualization.The
adjustedIndexcalculation (line 260) and slicing (line 263) correctly distributenumColumnsitems per row. Thereadonlyprop is properly threaded through tobuildOption.Note: Grid sizing
{{ sm: 12, md: 6 }}(line 264) relies on the implicit assumption that MUI's defaultmdbreakpoint (900px) matches the manualnumColumnsthreshold (line 47). This works but is fragile if theme breakpoints change. Consider dynamically computing Grid size fromnumColumnsfor robustness, though this is a minor concern.
…box to consider filtering
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (1)
23-23: Import LookupTable as a type, not a default value.
LookupTable.tsexports a named type. Use a type-only import to avoid runtime bloat and TS error.-import LookupTable from '../../../models/LookupTable'; +import type { LookupTable } from '../../../models/LookupTable';
♻️ Duplicate comments (4)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (4)
51-51: Correct property key usage for Select All — looks good.Now using
getPropertyValue(..., 'selectAll'). Matches schema.
52-66: Robust JSON parsing forlistDirection— approved.Safe parse with try/catch and array guard is correct.
98-101: Filter guard on placeholder items — approved.Protects against undefined labels and missing keys.
109-116: Select All should only toggle visible options; fix empty-set and indeterminate logic.
checkAllreplaces the entire selection and drops hidden selections.allInViewSelectedis true for empty filtered lists (vacuous every).indeterminateshould besome && !all.+ const visibleKeySet = useMemo(() => new Set(finalOptions?.map((o) => o.key) ?? []), [finalOptions]); + const selectedInViewCount = useMemo( + () => value.reduce((acc, item) => acc + (visibleKeySet.has(item.key) ? 1 : 0), 0), + [value, visibleKeySet] + ); - const allInViewSelected = useMemo(() => { - if (!finalOptions) return false; - return finalOptions.every((option) => checkedValuesLookup[option.key]); - }, [finalOptions, checkedValuesLookup]); - const someInViewSelected = useMemo(() => { - if (!finalOptions) return false; - return finalOptions.some((option) => checkedValuesLookup[option.key]); - }, [finalOptions, checkedValuesLookup]); + const allInViewSelected = finalOptions.length > 0 && selectedInViewCount === finalOptions.length; + const someInViewSelected = selectedInViewCount > 0 && !allInViewSelected;- const checkAll = () => { - if (value.length === finalOptions.length) { - setValue([]); - } else { - setValue(finalOptions.map((option) => ({ key: option.key, value_smv: option.value }))); - } - }; + const checkAll = () => { + if (allInViewSelected) { + // Unselect only visible ones + setValue(value.filter((item) => !visibleKeySet.has(item.key))); + } else { + // Select all visible (merge with existing) + const existing = new Set(value.map((v) => v.key)); + const additions = finalOptions + .filter((o) => !existing.has(o.key)) + .map((o) => ({ key: o.key, value_smv: o.value })); + setValue(value.concat(additions)); + } + };- indeterminate={someInViewSelected} + indeterminate={someInViewSelected && !allInViewSelected} checked={allInViewSelected}Also applies to: 144-150, 171-177
🧹 Nitpick comments (2)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (2)
103-108: Type the reduce initializer forcheckedValuesLookup.Initialize with a typed object to avoid implicit any and prototype pitfalls.
- }, {}); + }, Object.create(null) as Record<string, boolean>);
78-93: Tidy 2-column vertical reorder logic.Use
numRowsconsistently; current mix ofnumRowsandMath.ceil(.../2)is redundant.- const sortedOptions = []; + const sortedOptions: typeof finalOptions = []; const numRows = Math.ceil(finalOptions.length / 2); for (let i = 0; i < numRows; i++) { sortedOptions.push(finalOptions[i]); - if (finalOptions[i + numRows]) { - sortedOptions.push(finalOptions[i + Math.ceil(finalOptions.length / 2)]); - } + const right = finalOptions[i + numRows]; + if (right) sortedOptions.push(right); } finalOptions = sortedOptions;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (6)
ui/app/src/components/FormsEngine/types.ts (1)
ControlProps(23-30)ui/app/src/hooks/useWindowWidth.ts (1)
useWindowWidth(19-32)ui/app/src/components/FormsEngine/lib/formUtils.tsx (2)
isFieldReadOnly(764-766)getPropertyValue(749-755)ui/app/src/components/FormsEngine/dataSourceHooks/useKVPLoader.ts (2)
useKVPLoader(30-77)KVPLoaderItem(24-28)ui/app/src/components/SearchBar/SearchBar.tsx (1)
SearchBar(61-209)ui/app/src/models/LookupTable.ts (1)
LookupTable(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (1)
31-31: Use MUI Grid2 for thesizeprop.
Griddoesn't supportsize={{ sm, md }}; that's Grid v2 API. Import Grid2 and use it.-import Grid from '@mui/material/Grid'; +import Grid2 from '@mui/material/Grid2';- <Grid container spacing={2} sx={{ width: '100%' }}> + <Grid2 container spacing={2} sx={{ width: '100%' }}> - {Array.from({ length: 2 }).map((_, i) => ( - <Grid size={{ sm: 12, md: 6 }} key={i}> + {Array.from({ length: 2 }).map((_, i) => ( + <Grid2 size={{ sm: 12, md: 6 }} key={i}> ... - </Grid> + </Grid2> ))} - </Grid> + </Grid2>- <Grid container spacing={2} sx={{ width: '100%' }}> - {finalOptions?.map((option) => ( - <Grid size={{ sm: 12, md: 6 }} key={option.key}> + <Grid2 container spacing={2} sx={{ width: '100%' }}> + {finalOptions?.map((option) => ( + <Grid2 size={{ sm: 12, md: 6 }} key={option.key}> ... - </Grid> + </Grid2> ))} - </Grid> + </Grid2>- return ( - <Grid container className="checkbox-group-virtual-row" spacing={2} sx={{ width: '100%' }} style={style}> - {options.slice(adjustedIndex, adjustedIndex + numColumns).map((option) => ( - <Grid size={{ sm: 12, md: 6 }} key={option.key}> + return ( + <Grid2 container className="checkbox-group-virtual-row" spacing={2} sx={{ width: '100%' }} style={style}> + {options.slice(adjustedIndex, adjustedIndex + numColumns).map((option) => ( + <Grid2 size={{ sm: 12, md: 6 }} key={option.key}> ... - </Grid> + </Grid2> ))} - </Grid> + </Grid2>Also applies to: 121-126, 221-227, 271-277
⛔ Skipped due to learnings
Learnt from: jvega190 PR: craftercms/studio-ui#4580 File: ui/app/src/components/SitesGrid/SitesGrid.tsx:58-58 Timestamp: 2025-07-18T20:43:52.177Z Learning: In MUI 7, the `size` prop is not required for Grid components. When migrating from MUI 5 to MUI 7, removing the `item` prop from Grid components without adding a `size` prop is valid and acceptable. The `size` prop is only needed when you want to explicitly control the width of the grid item.Learnt from: jvega190 PR: craftercms/studio-ui#4580 File: ui/app/src/components/SitesGrid/SitesGrid.tsx:58-58 Timestamp: 2025-07-18T20:43:52.177Z Learning: In MUI v7, the `size` prop on Grid components is optional, not required. Grid items can exist without explicit sizing and will use their natural dimensions. When migrating from MUI v5 to v7, simply removing the `item` prop without adding a `size` prop is a valid approach when you want the Grid items to size naturally based on their content.
|
Outside diff range comments addressed |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (1)
110-117: Select All: scope to visible options, preserve existing selections, and fix a11y states
- checkAll currently clears all or overwrites selections outside the filter. Toggle should only affect visible options.
- allInViewSelected becomes true for an empty filtered list; indeterminate should exclude the all‑selected case.
Apply this consolidated patch:
- const allInViewSelected = useMemo(() => { - if (!finalOptions) return false; - return finalOptions.every((option) => checkedValuesLookup[option.key]); - }, [finalOptions, checkedValuesLookup]); - const someInViewSelected = useMemo(() => { - if (!finalOptions) return false; - return finalOptions.some((option) => checkedValuesLookup[option.key]); - }, [finalOptions, checkedValuesLookup]); + const visibleKeySet = useMemo(() => new Set(finalOptions?.map((o) => o.key) ?? []), [finalOptions]); + const selectedInViewCount = useMemo( + () => value.reduce((acc, item) => acc + (visibleKeySet.has(item.key) ? 1 : 0), 0), + [value, visibleKeySet] + ); + const allInViewSelected = Boolean(finalOptions?.length) && selectedInViewCount === (finalOptions?.length ?? 0); + const someInViewSelected = selectedInViewCount > 0 && !allInViewSelected; @@ - const checkAll = () => { - if (value.length === finalOptions.length) { - setValue([]); - } else { - setValue(finalOptions.map((option) => ({ key: option.key, value_smv: option.value }))); - } - }; + const checkAll = () => { + if (!finalOptions?.length) return; // nothing to toggle + if (allInViewSelected) { + // Unselect only visible ones + setValue(value.filter((item) => !visibleKeySet.has(item.key))); + } else { + // Select all visible, preserve existing selections outside the current filter + const existing = new Set(value.map((v) => v.key)); + const additions = finalOptions + .filter((o) => !existing.has(o.key)) + .map((o) => ({ key: o.key, value_smv: o.value })); + setValue(value.concat(additions)); + } + }; @@ - indeterminate={someInViewSelected} - checked={allInViewSelected} + indeterminate={someInViewSelected} + checked={allInViewSelected}Also applies to: 145-151, 172-178
🧹 Nitpick comments (4)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (4)
88-92: Use numRows consistently (clarity + micro-optimization)Replace repeated Math.ceil(finalOptions.length / 2) with numRows to avoid recomputation and improve readability.
- if (finalOptions[i + numRows]) { - sortedOptions.push(finalOptions[i + Math.ceil(finalOptions.length / 2)]); - } + if (finalOptions[i + numRows]) { + sortedOptions.push(finalOptions[i + numRows]); + }
98-103: Case‑insensitive search on keys for consistencyLabel comparison is case‑insensitive; key comparison is not. Normalize key too.
- return finalOptions?.filter((option) => { - const label = typeof option.value === 'string' ? option.value.toLowerCase() : ''; - return label.includes(lowerKeyword) || option.key?.includes(lowerKeyword); - }); + return finalOptions?.filter((option) => { + const label = typeof option.value === 'string' ? option.value.toLowerCase() : ''; + const keyLower = typeof option.key === 'string' ? option.key.toLowerCase() : ''; + return label.includes(lowerKeyword) || keyLower.includes(lowerKeyword); + });
104-109: Type the reducer accumulator for checkedValuesLookupAvoid implicit any on the accumulator; annotate as LookupTable.
- return value.reduce((acc, item) => { + return value.reduce((acc, item) => { acc[item.key] = true; return acc; - }, {}); + }, {} as LookupTable<boolean>);
241-243: Redundant key on inner FormControlLabelKey belongs on the mapped Grid item; the inner key is redundant. Safe to remove.
- <FormControlLabel - key={option.key} + <FormControlLabel
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (6)
ui/app/src/components/FormsEngine/types.ts (1)
ControlProps(23-30)ui/app/src/hooks/useWindowWidth.ts (1)
useWindowWidth(19-32)ui/app/src/components/FormsEngine/lib/formUtils.tsx (2)
isFieldReadOnly(764-766)getPropertyValue(749-755)ui/app/src/components/FormsEngine/dataSourceHooks/useKVPLoader.ts (2)
useKVPLoader(30-77)KVPLoaderItem(24-28)ui/app/src/components/SearchBar/SearchBar.tsx (1)
SearchBar(61-209)ui/app/src/models/LookupTable.ts (1)
LookupTable(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
ui/app/src/components/FormsEngine/controls/CheckboxGroup.tsx (2)
52-65: Good fix: robust listDirection parsingSafe JSON parsing with try/catch and array check looks solid. Dependency is scoped to field.properties.
124-126: Ignore “invalid size prop” warning
This file imports MUI v7’s updated Grid (Grid v2), which usessize(e.g.size={{ sm: 12, md: 6 }}) for breakpoints and no longer acceptsitem/xs/mdprops (legacy GridLegacy API). No changes needed. (mui.com)Likely an incorrect or invalid review comment.
…o feature/7418-checkbox-group # Conflicts: # ui/app/src/components/FormsEngine/lib/validators.ts
1 participant
craftercms/craftercms#7418
Summary by CodeRabbit
New Features
Improvements
Other