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 combining the checks for scoreA and scoreB into one comparison to reduce the number of conditional branches.

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.

[💡 performance]
The use of useMemo for normalizedSelectedTab is appropriate, but ensure that the performance benefit justifies the added complexity. If selectedTab changes infrequently, the memoization might not provide significant gains.

() => 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 consider if the logic is complex enough to warrant memoization. If the computation is trivial, memoization might add unnecessary complexity.

() => !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.

[⚠️ performance]
Ensure that sortSubmissionsByReviewScoreDesc is efficient for large datasets. Sorting can be costly, and if filteredReviews or filteredSubmitterReviews are large, this could impact performance.

? 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.

[💡 maintainability]
Consider adding a return type annotation to normalizePhaseName for better type safety and clarity.

.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) {

Choose a reason for hiding this comment

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

[⚠️ performance]
The addPhaseIdentifiers function is defined inside useMemo, which could lead to unnecessary re-creations of the function on every render. Consider moving it outside to improve performance.

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)
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 in the loop. Consider storing the result in a variable to avoid redundant computations.

))

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

return allowed
}, [challengePhases, phaseLookup])

Expand Down