Skip to content

Commit 4180031

Browse files
edenoclaude
edeno
and
claude
committed
fix(element): control CheckboxList/RadioList so selections mirror stored state
Port the uncontrolled->controlled fix from main (the fsgui epoch-accumulation class) to the modern branch. CheckboxList is active in v3 via ChannelMap's bad-channels; RadioList is used by the legacy form. Adds a canonical `value` prop, switches to checked/onChange, and fixes CheckboxList's memo comparison. Behavior note: a controlled radio fires onChange only on an actual change, so clicking the already-selected option is now a no-op (previously onClick re-fired redundantly); the affected test was updated to assert the new behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 5655274 commit 4180031

4 files changed

Lines changed: 106 additions & 13 deletions

File tree

‎src/__tests__/unit/components/CheckboxList.test.jsx‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,43 @@ describe('CheckboxList Component', () => {
264264
});
265265
});
266266

267+
describe('Controlled Selection (reflects state changes)', () => {
268+
it('updates checked state when the selection changes on re-render', () => {
269+
// Regression: with an uncontrolled `defaultChecked`, the DOM freezes at
270+
// its initial state and silently diverges from the stored/exported value
271+
// (the fsgui epoch-accumulation bug). The checkbox must mirror the prop.
272+
const { rerender } = render(
273+
<CheckboxList {...defaultProps} dataItems={['0', '1', '2']} defaultValue={[]} />
274+
);
275+
expect(screen.getByLabelText('1')).not.toBeChecked();
276+
277+
rerender(
278+
<CheckboxList {...defaultProps} dataItems={['0', '1', '2']} defaultValue={[1]} />
279+
);
280+
expect(screen.getByLabelText('1')).toBeChecked();
281+
});
282+
283+
it('clears a checkbox when it is removed from the selection on re-render', () => {
284+
const { rerender } = render(
285+
<CheckboxList {...defaultProps} dataItems={['0', '1', '2']} defaultValue={[1]} />
286+
);
287+
expect(screen.getByLabelText('1')).toBeChecked();
288+
289+
rerender(
290+
<CheckboxList {...defaultProps} dataItems={['0', '1', '2']} defaultValue={[]} />
291+
);
292+
expect(screen.getByLabelText('1')).not.toBeChecked();
293+
});
294+
295+
it('accepts the canonical `value` prop for the selection', () => {
296+
render(
297+
<CheckboxList {...defaultProps} dataItems={['0', '1', '2']} value={[2]} />
298+
);
299+
expect(screen.getByLabelText('0')).not.toBeChecked();
300+
expect(screen.getByLabelText('2')).toBeChecked();
301+
});
302+
});
303+
267304
describe('User Interactions', () => {
268305
it('should call updateFormArray when checkbox is clicked', async () => {
269306
const user = userEvent.setup();

‎src/__tests__/unit/components/RadioList.test.jsx‎

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,31 @@ describe('RadioList Component', () => {
300300
});
301301
});
302302

303+
describe('Controlled Selection (reflects state changes)', () => {
304+
it('updates the checked radio when the selection changes on re-render', () => {
305+
// Regression: an uncontrolled `defaultChecked` freezes the DOM at its
306+
// initial state and silently diverges from the stored value. The radio
307+
// must mirror the prop.
308+
const { rerender } = render(
309+
<RadioList {...defaultProps} dataItems={['0', '1', '2']} defaultValue="" />
310+
);
311+
expect(screen.getByLabelText('1')).not.toBeChecked();
312+
313+
rerender(
314+
<RadioList {...defaultProps} dataItems={['0', '1', '2']} defaultValue="1" />
315+
);
316+
expect(screen.getByLabelText('1')).toBeChecked();
317+
});
318+
319+
it('accepts the canonical `value` prop and matches a numeric selection to string options', () => {
320+
render(
321+
<RadioList {...defaultProps} dataItems={['0', '1', '2']} value={1} />
322+
);
323+
expect(screen.getByLabelText('0')).not.toBeChecked();
324+
expect(screen.getByLabelText('1')).toBeChecked();
325+
});
326+
});
327+
303328
describe('User Interactions', () => {
304329
it('should call updateFormData when radio is clicked', async () => {
305330
const user = userEvent.setup();
@@ -413,7 +438,7 @@ describe('RadioList Component', () => {
413438
);
414439
});
415440

416-
it('should handle clicking already-checked radio', async () => {
441+
it('does not re-fire updateFormData when the already-checked radio is clicked', async () => {
417442
const user = userEvent.setup();
418443
const mockUpdate = vi.fn();
419444

@@ -431,8 +456,12 @@ describe('RadioList Component', () => {
431456

432457
await user.click(radio);
433458

434-
// Still calls updateFormData (radio behavior)
435-
expect(mockUpdate).toHaveBeenCalledTimes(1);
459+
// Now that the radio is controlled, onChange fires only when the selection
460+
// actually changes; clicking the already-selected option is a no-op (the
461+
// stored value already matches). Previously the component used onClick,
462+
// which re-fired updateFormData redundantly with the same value.
463+
expect(mockUpdate).not.toHaveBeenCalled();
464+
expect(radio).toBeChecked();
436465
});
437466

438467
it('should handle multiple radio clicks (only last one checked)', async () => {

‎src/element/CheckboxList.jsx‎

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ const CheckboxListComponent = (prop) => {
2222
dataItems,
2323
objectKind,
2424
placeholder,
25+
value,
2526
defaultValue,
2627
updateFormArray,
2728
metaData,
@@ -30,15 +31,22 @@ const CheckboxListComponent = (prop) => {
3031

3132
const id = useStableId(providedId, 'checkbox-list');
3233

34+
// Controlled selection: render the values actually stored in form state so
35+
// the checkboxes always mirror it. Otherwise the DOM and the exported data
36+
// can silently diverge (boxes show one thing, the YAML exports another).
37+
// `value` is the canonical prop; some call sites still pass the selected
38+
// array as `defaultValue`, so accept either.
39+
const selectedValues = value !== undefined ? value : defaultValue;
40+
3341
const onChecked = (e) => {
3442
const { target } = e;
35-
const value = parseInt(target.value, 10);
43+
const numericValue = parseInt(target.value, 10);
3644
// const values = Array.from(
3745
// target.parentElement.querySelectorAll('input[type="checkbox"]:checked')
3846
// ).map((a) => parseInt(a.value, 10));
3947

4048
const { nameValue, keyValue, index } = metaData;
41-
updateFormArray(nameValue, value, keyValue, index, e.target.checked);
49+
updateFormArray(nameValue, numericValue, keyValue, index, e.target.checked);
4250
};
4351

4452
return (
@@ -61,8 +69,8 @@ const CheckboxListComponent = (prop) => {
6169
id={`${id}-${dataItemIndex}`}
6270
name={`${name}-${id}`}
6371
value={dataItem}
64-
defaultChecked={defaultValue.includes(stringToInteger(dataItem))}
65-
onClick={onChecked}
72+
checked={selectedValues.includes(stringToInteger(dataItem))}
73+
onChange={onChecked}
6674
required={required && dataItemIndex === 0 ? true : undefined}
6775
/>
6876
<label htmlFor={`${id}-${dataItemIndex}`}> {dataItem}</label>
@@ -83,6 +91,7 @@ const CheckboxListComponent = (prop) => {
8391

8492
CheckboxListComponent.propTypes = {
8593
title: PropTypes.string.isRequired,
94+
value: PropTypes.instanceOf(Array),
8695
defaultValue: PropTypes.instanceOf(Array),
8796
dataItems: PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string, PropTypes.number])),
8897
id: PropTypes.string,
@@ -97,6 +106,7 @@ CheckboxListComponent.propTypes = {
97106

98107
CheckboxListComponent.defaultProps = {
99108
id: undefined,
109+
value: undefined,
100110
defaultValue: [],
101111
dataItems: [],
102112
placeholder: '',
@@ -107,8 +117,15 @@ CheckboxListComponent.defaultProps = {
107117
};
108118

109119
const arePropsEqual = (prevProps, nextProps) => {
120+
// Compare the effective selection (controlled `value`, else `defaultValue`)
121+
// by reference so a state change always triggers a re-render — otherwise the
122+
// controlled checkboxes could freeze out of sync with form state.
123+
const prevSelected =
124+
prevProps.value !== undefined ? prevProps.value : prevProps.defaultValue;
125+
const nextSelected =
126+
nextProps.value !== undefined ? nextProps.value : nextProps.defaultValue;
110127
return (
111-
prevProps.defaultValue === nextProps.defaultValue &&
128+
prevSelected === nextSelected &&
112129
prevProps.dataItems === nextProps.dataItems &&
113130
prevProps.name === nextProps.name &&
114131
prevProps.required === nextProps.required

‎src/element/RadioList.jsx‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ const RadioList = (prop) => {
2323
dataItems,
2424
objectKind,
2525
placeholder,
26+
value,
2627
defaultValue,
2728
updateFormData,
2829
metaData,
@@ -32,14 +33,21 @@ const RadioList = (prop) => {
3233

3334
const id = useStableId(providedId, 'radio-list');
3435

36+
// Controlled selection: render the value actually stored in form state so the
37+
// radios always mirror it (otherwise the DOM and the stored/exported data can
38+
// silently diverge). `value` is the canonical prop; some call sites still pass
39+
// the selection as `defaultValue`, so accept either. Compare as strings so a
40+
// numeric stored value (e.g. camera_id) matches its string dataItem option.
41+
const selectedValue = value !== undefined ? value : defaultValue;
42+
3543
const onChecked = (e) => {
3644
const { target } = e;
37-
const { value } = target;
45+
const { value: targetValue } = target;
3846
let radioValue;
3947
if (type === 'number') {
40-
radioValue = parseInt(value, 10);
48+
radioValue = parseInt(targetValue, 10);
4149
}
42-
else {radioValue = value;}
50+
else {radioValue = targetValue;}
4351
const { nameValue, keyValue, index } = metaData;
4452

4553
updateFormData(nameValue, radioValue, keyValue, index);
@@ -65,8 +73,8 @@ const RadioList = (prop) => {
6573
id={`${id}-${dataItemIndex}`}
6674
name={`${name}-${id}`}
6775
value={dataItem}
68-
defaultChecked={defaultValue === dataItem}
69-
onClick={onChecked}
76+
checked={String(selectedValue) === String(dataItem)}
77+
onChange={onChecked}
7078
required={required || undefined}
7179
/>
7280
<label htmlFor={`${id}-${dataItemIndex}`}> {dataItem}</label>
@@ -87,6 +95,7 @@ const RadioList = (prop) => {
8795

8896
RadioList.propTypes = {
8997
title: PropTypes.string.isRequired,
98+
value: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
9099
defaultValue: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
91100
dataItems: PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string, PropTypes.number])),
92101
id: PropTypes.string,
@@ -101,6 +110,7 @@ RadioList.propTypes = {
101110

102111
RadioList.defaultProps = {
103112
id: undefined,
113+
value: undefined,
104114
defaultValue: '',
105115
dataItems: [],
106116
placeholder: '',

0 commit comments

Comments
 (0)