Feat/ Fix for page crashing when editing a scorecard #1287
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ) | ||
| {options.map((option, index) => ( | ||
| <option | ||
| key={`${option.value ?? option.label ?? index}-${index}`} |
There was a problem hiding this comment.
[correctness]
Using option.value ?? '' for the value attribute may lead to unexpected behavior if option.value is null or undefined. Consider explicitly handling these cases to ensure the value attribute is always a valid string.
| control: form.control, | ||
| defaultValue: [], | ||
| name: props.fieldName, | ||
| }) as { weight?: number | string | null }[] | undefined |
There was a problem hiding this comment.
[correctness]
Casting useWatch result to { weight?: number | string | null }[] | undefined could lead to runtime errors if the structure of the watched fields changes. Consider adding runtime validation to ensure the structure matches expectations.
| const weightsSum = useMemo( | ||
| () => fields.reduce( | ||
| (sum: number, field: { weight?: string | number | null }) => ( | ||
| Number(field?.weight ?? 0) + sum |
There was a problem hiding this comment.
[correctness]
Using Number(field?.weight ?? 0) is safe here, but be aware that Number will convert non-numeric strings to NaN, which might not be the intended behavior. Consider validating or sanitizing field.weight before conversion if there's a risk of unexpected input.
| QA: 'QUALITY_ASSURANCE', | ||
| } | ||
|
|
||
| const normalizeTrackOptionValue = (track: useFetchChallengeTracksProps['challengeTracks'][number]): string => { |
There was a problem hiding this comment.
[❗❗ correctness]
The normalizeTrackOptionValue function uses track.track and track.name without checking if these properties exist on the track object. Consider adding type checks or default values to prevent runtime errors if these properties are undefined.
| const { challengeTypes }: useFetchChallengeTypesProps = useFetchChallengeTypes() | ||
| const { getValues, setValue } = form | ||
| const normalizeValue = useCallback(( | ||
| value: string | number | boolean | null | undefined, |
There was a problem hiding this comment.
[correctness]
The normalizeValue function converts any input to a string and trims it. This might lead to unexpected behavior if the input is a non-string type, such as a number or boolean. Consider handling different types explicitly or documenting the expected input types.
| ), [challengeTracks]) | ||
|
|
||
| const fallbackChallengeTrack = useMemo( | ||
| () => challengeTrackOptions.find(option => option.value)?.value, |
There was a problem hiding this comment.
[correctness]
The fallbackChallengeTrack logic assumes that the first active track option is always valid. This might lead to incorrect fallback behavior if the options are not properly filtered or sorted. Consider adding validation or sorting logic to ensure the correct fallback is selected.
| ), [challengeTypes]) | ||
|
|
||
| const fallbackChallengeType = useMemo( | ||
| () => challengeTypeOptions.find(option => option.value)?.value, |
There was a problem hiding this comment.
[correctness]
The fallbackChallengeType logic assumes that the first active type option is always valid. Similar to fallbackChallengeTrack, this might lead to incorrect fallback behavior if the options are not properly filtered or sorted. Consider adding validation or sorting logic to ensure the correct fallback is selected.
| const shouldNormalizeType = useRef(true) | ||
|
|
||
| useEffect(() => { | ||
| if (!shouldNormalizeTrack.current) { |
There was a problem hiding this comment.
[💡 maintainability]
The use of useRef for shouldNormalizeTrack and shouldNormalizeType flags is correct but could be improved by ensuring these flags are reset appropriately after normalization logic is executed. Consider adding more explicit control over these flags to prevent unintended behavior.
Feat/ Fix for page crashing when editing a scorecard