Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,115 @@ interface Props {
isActiveChallenge: boolean
}

const normalizeTabLabel = (value?: string): string => (
value
? value
.toLowerCase()
.replace(/[^a-z]/g, '')
: ''
)

const parseScoreValue = (value: unknown): number | undefined => {
if (typeof value === 'number') {
return Number.isFinite(value) ? value : undefined
}

if (typeof value === 'string') {
const trimmed = value.trim()
if (!trimmed.length) {
return undefined
}

const parsed = Number.parseFloat(trimmed)
return Number.isFinite(parsed) ? parsed : undefined
}

return undefined
}

const resolveSubmissionReviewScore = (submission: SubmissionInfo): number | undefined => {
const reviewResultScores = Array.isArray(submission.reviews)
? submission.reviews
.map(review => parseScoreValue(review?.score))
.filter((score): score is number => typeof score === 'number')
: []

if (reviewResultScores.length) {
const total = reviewResultScores.reduce((sum, score) => sum + score, 0)
return total / reviewResultScores.length
}

const aggregateScore = parseScoreValue(submission.aggregateScore)
if (aggregateScore !== undefined) {
return aggregateScore
}

const finalScore = parseScoreValue(submission.review?.finalScore)
if (finalScore !== undefined) {
return finalScore
}

const initialScore = parseScoreValue(submission.review?.initialScore)
if (initialScore !== undefined) {
return initialScore
}

return undefined
}

type SubmissionScoreEntry = {
index: number
score?: number
submission: SubmissionInfo
}

const sortSubmissionsByReviewScoreDesc = (rows: SubmissionInfo[]): SubmissionInfo[] => {
const entries: SubmissionScoreEntry[] = rows.map((submission, index) => ({
index,
score: resolveSubmissionReviewScore(submission),
submission,
}))

entries.sort((a: SubmissionScoreEntry, b: SubmissionScoreEntry) => {

Choose a reason for hiding this comment

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

[⚠️ performance]
The sorting logic in sortSubmissionsByReviewScoreDesc could be optimized by using a single comparison function. Consider using localeCompare for string comparisons or directly comparing numbers to simplify the logic.

const scoreA: number | undefined = a.score
const scoreB: number | undefined = b.score
const indexA: number = a.index
const indexB: number = b.index

if (scoreA === undefined && scoreB === undefined) {
return indexA - indexB
}

if (scoreA === undefined) {
return 1
}

if (scoreB === undefined) {
return -1
}

if (scoreB !== scoreA) {
return scoreB - scoreA
}

return indexA - indexB
})

return entries.map(entry => entry.submission)
}

export const TabContentReview: FC<Props> = (props: Props) => {
const selectedTab = props.selectedTab
const providedReviews = props.reviews
const providedSubmitterReviews = props.submitterReviews
const normalizedSelectedTab = useMemo(

Choose a reason for hiding this comment

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

[💡 maintainability]
The useMemo hook for normalizedSelectedTab is used correctly, but ensure that the normalizeTabLabel function is pure and does not have side effects, as it could lead to unexpected behavior.

() => normalizeTabLabel(selectedTab),
[selectedTab],
)
const shouldSortReviewTabByScore = useMemo(

Choose a reason for hiding this comment

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

[💡 performance]
The useMemo hook for shouldSortReviewTabByScore is used correctly, but be cautious about the dependency array. If props.isActiveChallenge or normalizedSelectedTab change frequently, it could lead to unnecessary re-renders.

() => !props.isActiveChallenge && normalizedSelectedTab === 'review',
[normalizedSelectedTab, props.isActiveChallenge],
)
const {
challengeInfo,
challengeSubmissions: backendChallengeSubmissions,
Expand Down Expand Up @@ -546,13 +651,27 @@ export const TabContentReview: FC<Props> = (props: Props) => {
},
[resolvedSubmitterReviews],
)
const reviewerRowsForReviewTab = useMemo(
() => (shouldSortReviewTabByScore

Choose a reason for hiding this comment

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

[💡 maintainability]
The useMemo hook for reviewerRowsForReviewTab and submitterRowsForReviewTab is used correctly, but ensure that sortSubmissionsByReviewScoreDesc is a pure function without side effects to avoid unexpected behavior.

? sortSubmissionsByReviewScoreDesc(filteredReviews)
: filteredReviews),
[filteredReviews, shouldSortReviewTabByScore],
)
const submitterRowsForReviewTab = useMemo(
() => (shouldSortReviewTabByScore
? sortSubmissionsByReviewScoreDesc(filteredSubmitterReviews)
: filteredSubmitterReviews),
[filteredSubmitterReviews, shouldSortReviewTabByScore],
)
const hideHandleColumn = props.isActiveChallenge
&& actionChallengeRole === REVIEWER

// show loading ui when fetching data
const isSubmitterView = actionChallengeRole === SUBMITTER
&& selectedTab !== APPROVAL
const reviewRows = isSubmitterView ? filteredSubmitterReviews : filteredReviews
const reviewRows = isSubmitterView
? (shouldSortReviewTabByScore ? submitterRowsForReviewTab : filteredSubmitterReviews)
: (shouldSortReviewTabByScore ? reviewerRowsForReviewTab : filteredReviews)

if (props.isLoadingReview) {
return <TableLoading />
Expand Down Expand Up @@ -596,14 +715,14 @@ export const TabContentReview: FC<Props> = (props: Props) => {

return isSubmitterView ? (
<TableReviewForSubmitter
datas={filteredSubmitterReviews}
datas={reviewRows}
isDownloading={props.isDownloading}
downloadSubmission={props.downloadSubmission}
mappingReviewAppeal={props.mappingReviewAppeal}
/>
) : (
<TableReview
datas={filteredReviews}
datas={reviewRows}
isDownloading={props.isDownloading}
downloadSubmission={props.downloadSubmission}
mappingReviewAppeal={props.mappingReviewAppeal}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ interface Props {
className?: string
}

const normalizePhaseName = (name?: string): string => (name ? name.trim()

Choose a reason for hiding this comment

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

[💡 style]
Consider using name?.trim().toLowerCase() ?? '' instead of the ternary operator for a more concise and idiomatic approach to handle optional chaining and default values.

.toLowerCase() : '')
const SUBMISSION_PHASE_NAMES = new Set(['submission', 'topgear submission'])
const REGISTRATION_PHASE_NAME = 'registration'

// Helpers to keep UI hooks simple and under complexity limits

// Compute a Set of this member's reviewer resource IDs (excluding iterative roles)
Expand Down Expand Up @@ -1089,23 +1094,41 @@ export const ChallengeDetailsPage: FC<Props> = (props: Props) => {
return allowed
}

challengePhases.forEach(phase => {
if (!phase?.isOpen || !phase.predecessor) {
const addPhaseIdentifiers = (phase?: BackendPhase): void => {
if (!phase) {
return
}

allowed.add(phase.predecessor)
if (phase.id) {
allowed.add(phase.id)
}

const predecessorPhase = phaseLookup.get(phase.predecessor)
if (predecessorPhase?.id) {
allowed.add(predecessorPhase.id)
if (phase.phaseId) {
allowed.add(phase.phaseId)
}
}

if (predecessorPhase?.phaseId) {
allowed.add(predecessorPhase.phaseId)
challengePhases.forEach(phase => {
if (!phase?.isOpen || !phase.predecessor) {
return
}

allowed.add(phase.predecessor)

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The allowed.add(phase.predecessor) call is duplicated in the challengePhases.forEach loop and the addPhaseIdentifiers function. Consider removing the duplication to improve maintainability.

addPhaseIdentifiers(phaseLookup.get(phase.predecessor))
})

const hasSubmissionVariantOpen = challengePhases.some(phase => (
phase?.isOpen && SUBMISSION_PHASE_NAMES.has(normalizePhaseName(phase.name))

Choose a reason for hiding this comment

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

[⚠️ performance]
The normalizePhaseName function is called multiple times for the same phase name. Consider normalizing the phase name once and storing it in a variable to improve performance.

))

if (hasSubmissionVariantOpen) {
challengePhases.forEach(phase => {
if (normalizePhaseName(phase?.name) === REGISTRATION_PHASE_NAME) {
addPhaseIdentifiers(phase)
}
})
}

return allowed
}, [challengePhases, phaseLookup])

Expand Down