Repository navigation
[TR] The option "None of the above" is not unticked in case other option is selected - #1667
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the "None of the above" selection logic in the dynamic form component and adds corresponding unit tests. The refactoring replaces the use of RxJS pairwise and startWith operators with individual control subscriptions, and fixes index checks. The reviewer recommends subscribing to the parent FormGroup's valueChanges once instead of subscribing to each control individually to prevent potential memory leaks and improve efficiency. Additionally, the reviewer suggests adding a type check in getNoneOptionIndex to avoid a potential runtime TypeError when calling .toLowerCase() on non-string values.
| private applyNoneLogic(control: FormGroup, noneKey: string) { | ||
| Object.keys(control.controls).forEach(key => { | ||
| control | ||
| .get(key) | ||
| ?.valueChanges.pipe(takeUntil(this.destroy$)) | ||
| .subscribe(checked => { | ||
| if (!checked) { | ||
| return; | ||
| } | ||
|
|
||
| if (key === noneKey) { | ||
| Object.keys(control.controls).forEach(k => { | ||
| if (k !== noneKey) { | ||
| control.get(k)?.setValue(false, { emitEvent: false }); | ||
| } | ||
| }); | ||
| } else if (control.get(noneKey)?.value === true) { | ||
| control.get(noneKey)?.setValue(false, { emitEvent: false }); | ||
| } | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Subscribing to each individual control's valueChanges in a loop creates multiple subscriptions and is less efficient. It can also lead to memory leaks if the form controls are recreated or updated. A cleaner and more idiomatic Angular/RxJS approach is to subscribe to the parent FormGroup's valueChanges once, and keep track of the previous value using a local variable to detect changes.
private applyNoneLogic(control: FormGroup, noneKey: string) {
let previousValue = { ...control.value };
control.valueChanges
.pipe(takeUntil(this.destroy$))
.subscribe(currentValue => {
const changedKey = Object.keys(currentValue).find(
key => currentValue[key] === true && !previousValue[key]
);
previousValue = { ...currentValue };
if (!changedKey) {
return;
}
if (changedKey === noneKey) {
const newValue = { ...currentValue };
Object.keys(newValue).forEach(key => {
if (key !== noneKey) {
newValue[key] = false;
}
});
control.setValue(newValue, { emitEvent: false });
previousValue = newValue;
} else if (currentValue[noneKey] === true) {
control.patchValue({ [noneKey]: false }, { emitEvent: false });
previousValue = { ...currentValue, [noneKey]: false };
}
});
}| private getNoneOptionIndex(options: OptionType[] | undefined): number | null { | ||
| if (!options) return null; | ||
| return options.findIndex(option => | ||
| const index = options.findIndex(option => | ||
| this.getOptionValue(option)?.toLowerCase().includes('none') | ||
| ); | ||
| return index !== -1 ? index : null; | ||
| } |
There was a problem hiding this comment.
The getOptionValue(option) method can return a non-string value (e.g., an object or undefined). Calling .toLowerCase() directly on a non-string value will cause a runtime TypeError. We should ensure the value is a string before calling .toLowerCase().
private getNoneOptionIndex(options: OptionType[] | undefined): number | null {
if (!options) return null;
const index = options.findIndex(option => {
const value = this.getOptionValue(option);
return typeof value === 'string' && value.toLowerCase().includes('none');
});
return index !== -1 ? index : null;
}
Fix "None of the above" option selection