Skip to content

Feat/ Fix for page crashing when editing a scorecard #1287

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 2, 2025
Merged

Feat/ Fix for page crashing when editing a scorecard #1287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d933676
Fix performance issue on scorecard editor page
jmgasper Nov 2, 2025
147b220
Fix for page crashing when editing a scorecard
jmgasper Nov 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 38 additions & 24 deletions src/apps/review/src/pages/scorecards/EditScorecardPage/components/BasicSelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,37 +1,51 @@
import { omit } from 'lodash'
import { FC, SelectHTMLAttributes } from 'react'
import { forwardRef, SelectHTMLAttributes } from 'react'
import classNames from 'classnames'

interface BasicSelectProps<T> extends SelectHTMLAttributes<T> {
options: { label: string; value: string|boolean|number }[];
mapValue?: (value: any) => string;
placeholder?: string;
}

const BasicSelect: FC<BasicSelectProps<any>> = props => (
<select
{...omit(props, 'options')}
className={
classNames(
props.className,
!props.value && `${props.value}` !== 'false' && 'empty',
)
}
>
<option
disabled
value=''
const BasicSelect = forwardRef<HTMLSelectElement, BasicSelectProps<any>>((
props,
ref,
) => {
const { className, options, placeholder, value, mapValue: _mapValue, ...rest } = props

const normalizedValue = value === null || value === undefined ? '' : value

return (
<select
ref={ref}
{...rest}
className={
classNames(
className,
!normalizedValue && `${normalizedValue}` !== 'false' && 'empty',
)
}
value={normalizedValue as any}
>
{props.placeholder}
</option>
{props.options.map(option => (
<option
key={`${option.value}`}
value={`${option.value}`}
key='placeholder-option'
disabled
value=''
>
{option.label}
{placeholder}
</option>
))}
</select>
)
{options.map((option, index) => (
<option
key={`${option.value ?? option.label ?? index}-${index}`}
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

value={`${option.value ?? ''}`}
>
{option.label}
</option>
))}
</select>
)
})

BasicSelect.displayName = 'BasicSelect'

export default BasicSelect
26 changes: 20 additions & 6 deletions src/apps/review/src/pages/scorecards/EditScorecardPage/components/CalculatedWeightsSum.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { FC } from 'react'
import { useFormContext } from 'react-hook-form'
import { FC, useMemo } from 'react'
import { useFormContext, useWatch } from 'react-hook-form'
import classNames from 'classnames'

import styles from './CalculatedWeightsSum.module.scss'
Expand All @@ -13,11 +13,25 @@ interface CalculatedWeightsSumProps {

const CalculatedWeightsSum: FC<CalculatedWeightsSumProps> = props => {
const form = useFormContext()
const fields = form.watch(props.fieldName)
const watchedFields = useWatch({
control: form.control,
defaultValue: [],
name: props.fieldName,
}) as { weight?: number | string | null }[] | undefined
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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 = fields.reduce(
(sum: number, field: { weight: string | number | undefined }) => (Number(field.weight) || 0) + sum,
0,
const fields = useMemo(
() => (Array.isArray(watchedFields) ? watchedFields : []),
[watchedFields],
)

const weightsSum = useMemo(
() => fields.reduce(
(sum: number, field: { weight?: string | number | null }) => (
Number(field?.weight ?? 0) + sum
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

),
0,
),
[fields],
)

return (
Expand Down
148 changes: 119 additions & 29 deletions src/apps/review/src/pages/scorecards/EditScorecardPage/components/ScorecardInfoForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as yup from 'yup'
import { FC, useEffect, useMemo } from 'react'
import { FC, useCallback, useEffect, useMemo, useRef } from 'react'
import { useFormContext } from 'react-hook-form'
import classNames from 'classnames'

Expand Down Expand Up @@ -70,8 +70,30 @@ const toChallengeTrackLabel = (value: string): string => (
)

const legacyChallengeTrackMap: Record<string, string> = {
DEVELOPMENT: 'DEVELOP',
QUALITY_ASSURANCE: 'QA',
DEVELOPMENT: 'DEVELOPMENT',
DEVELOP: 'DEVELOPMENT',
QUALITY_ASSURANCE: 'QUALITY_ASSURANCE',
QA: 'QUALITY_ASSURANCE',
}

const normalizeTrackOptionValue = (track: useFetchChallengeTracksProps['challengeTracks'][number]): string => {
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ 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.

if (track.track) {
return track.track
}

const normalizedName = track.name
?.replace(/\s+/g, '_')
.toUpperCase()

if (normalizedName && legacyChallengeTrackMap[normalizedName]) {
return legacyChallengeTrackMap[normalizedName]
}

if (track.name) {
return normalizedName || track.name
}

return track.id
}

const normalizeChallengeTrackValue = (
Expand All @@ -82,21 +104,23 @@ const normalizeChallengeTrackValue = (

const directMatch = tracks.find(track => track.track === value && track.isActive)
if (directMatch) {
return directMatch.track
return normalizeTrackOptionValue(directMatch)
}

const mappedValue = legacyChallengeTrackMap[value]
if (mappedValue) {
const mappedMatch = tracks.find(track => track.track === mappedValue && track.isActive)
const mappedMatch = tracks.find(track => (
normalizeTrackOptionValue(track) === mappedValue && track.isActive
))
if (mappedMatch) {
return mappedMatch.track
return normalizeTrackOptionValue(mappedMatch)
}
}

const normalizedLabel = toChallengeTrackLabel(value)
const nameMatch = tracks.find(track => track.name === normalizedLabel && track.isActive)
if (nameMatch) {
return nameMatch.track
return normalizeTrackOptionValue(nameMatch)
}

const uppercaseValue = value
Expand All @@ -109,23 +133,41 @@ const normalizeChallengeTrackValue = (
&& track.isActive
))

return fallbackMatch?.track
return fallbackMatch ? normalizeTrackOptionValue(fallbackMatch) : undefined
}

const ScorecardInfoForm: FC = () => {
const form = useFormContext()
const { challengeTracks }: useFetchChallengeTracksProps = useFetchChallengeTracks()
const { challengeTypes }: useFetchChallengeTypesProps = useFetchChallengeTypes()
const { getValues, setValue } = form
const normalizeValue = useCallback((
value: string | number | boolean | null | undefined,
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

): string | undefined => {
if (value === null || value === undefined) {
return undefined
}

const normalized = String(value).trim()

return normalized.length ? normalized : undefined
}, [])

const challengeTrackOptions = useMemo(() => (
challengeTracks
.filter(track => track.isActive)
.map(track => ({
label: track.name,
value: track.track,
value: normalizeTrackOptionValue(track),
}))
), [challengeTracks])

const fallbackChallengeTrack = useMemo(
() => challengeTrackOptions.find(option => option.value)?.value,
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

[challengeTrackOptions],
)
const shouldNormalizeTrack = useRef(true)

const challengeTypeOptions = useMemo(() => (
challengeTypes
.filter(type => type.isActive)
Expand All @@ -135,49 +177,97 @@ const ScorecardInfoForm: FC = () => {
}))
), [challengeTypes])

const fallbackChallengeType = useMemo(
() => challengeTypeOptions.find(option => option.value)?.value,
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

[challengeTypeOptions],
)
const shouldNormalizeType = useRef(true)

useEffect(() => {
if (!shouldNormalizeTrack.current) {
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 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.

return
}

if (!challengeTrackOptions.length) {
return
}

const currentValue: string = form.getValues('challengeTrack') as string
const currentValue = normalizeValue(getValues('challengeTrack') as string)
const isCurrentValid: boolean = !!currentValue
&& challengeTrackOptions.some(option => option.value === currentValue)
&& challengeTrackOptions.some(option => (
normalizeValue(option.value) === currentValue
))

if (currentValue && !isCurrentValid) {
const normalizedValue = normalizeChallengeTrackValue(currentValue, challengeTracks)
if (normalizedValue) {
form.setValue('challengeTrack', normalizedValue, {
const normalizedValue = normalizeValue(
normalizeChallengeTrackValue(currentValue, challengeTracks),
)

if (normalizedValue && normalizedValue !== currentValue) {
setValue('challengeTrack', normalizedValue, {
shouldDirty: false,
shouldValidate: true,
})
return
}

return
}

if (!isCurrentValid) {
form.setValue('challengeTrack', challengeTrackOptions[0].value, {
shouldDirty: false,
shouldValidate: true,
})
if (!isCurrentValid && fallbackChallengeTrack) {
const normalizedFallback = normalizeValue(fallbackChallengeTrack)

if (normalizedFallback && normalizedFallback !== currentValue) {
setValue('challengeTrack', normalizedFallback, {
shouldDirty: false,
shouldValidate: true,
})
}
}
}, [challengeTrackOptions, challengeTracks, form])

shouldNormalizeTrack.current = false
}, [
challengeTrackOptions,
challengeTracks,
fallbackChallengeTrack,
getValues,
normalizeValue,
setValue,
])

useEffect(() => {
if (!shouldNormalizeType.current) {
return
}

if (!challengeTypeOptions.length) {
return
}

const currentChallengeType: string = form.getValues('challengeType') as string
const currentChallengeType = normalizeValue(getValues('challengeType') as string)
const partOfCategories: { label: string; value: string } | undefined
= challengeTypeOptions.find(item => item.value === currentChallengeType)
if ((!partOfCategories || !currentChallengeType) && challengeTypeOptions.length > 0) {
form.setValue('challengeType', challengeTypeOptions[0].value, {
shouldDirty: false,
shouldValidate: true,
})
= challengeTypeOptions.find(item => (
normalizeValue(item.value) === currentChallengeType
))

if ((!partOfCategories || !currentChallengeType) && fallbackChallengeType) {
const normalizedFallback = normalizeValue(fallbackChallengeType)

if (normalizedFallback && normalizedFallback !== currentChallengeType) {
setValue('challengeType', normalizedFallback, {
shouldDirty: false,
shouldValidate: true,
})
}
}
}, [challengeTypeOptions, form])

shouldNormalizeType.current = false
}, [
challengeTypeOptions,
fallbackChallengeType,
getValues,
normalizeValue,
setValue,
])

return (
<div className={classNames(styles.grayWrapper, styles.scorecardInfo)}>
Expand Down