Topgear Iterative Reviewer visibility fix #1301
Conversation
…sion phases open - prioritise submission.
…challenges and better group searching in system admin app
Hotfixes for minor issues
Topgear Iterative Reviewer / submission list fix
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| usersMapping, | ||
| ) | ||
|
|
||
| const filteredGroups = useMemo(() => { |
There was a problem hiding this comment.
[correctness]
Using useMemo for filteredGroups is appropriate to avoid unnecessary recalculations, but ensure that groups is not being mutated elsewhere in the component, as it could lead to unexpected results.
| return id.includes(normalized) || name.includes(normalized) | ||
| }) | ||
| }, [groups, searchTerm]) | ||
| const hasSearchTerm = useMemo( |
There was a problem hiding this comment.
[💡 performance]
The useMemo for hasSearchTerm is not necessary here since the computation is trivial. Removing it could simplify the code without impacting performance.
| [myResources], | ||
| ) | ||
| const allowTopgearSubmissionList = useMemo( | ||
| () => actionChallengeRole !== SUBMITTER || hasIterativeReviewerRole, |
There was a problem hiding this comment.
[💡 performance]
The useMemo hook is used here to determine allowTopgearSubmissionList, but the logic is simple and doesn't involve heavy computation. Consider removing useMemo to simplify the code unless there's a specific performance concern.
| }: ChallengeDetailContextModel = useContext(ChallengeDetailContext) | ||
| const { actionChallengeRole }: useRoleProps = useRole() | ||
| const hasIterativeReviewerRole = useMemo( | ||
| () => myResources.some( |
There was a problem hiding this comment.
[💡 performance]
The useMemo hook is used to compute hasIterativeReviewerRole, but the computation is straightforward and not computationally expensive. Consider removing useMemo to simplify the code unless there's a specific performance concern.
| timeLeftColor: displayTimeLeftColor, | ||
| timeLeftStatus: displayTimeLeftStatus, | ||
| }: PhaseDisplayInfo = useMemo( | ||
| () => computePhaseDisplayInfo(props.challengeInfo), |
There was a problem hiding this comment.
[💡 performance]
The useMemo hook is used here to compute PhaseDisplayInfo, but if computePhaseDisplayInfo is not computationally expensive, it might be unnecessary. Consider whether memoization is needed, as it adds complexity and can lead to stale values if dependencies are not managed correctly.
| function selectPhaseForDisplay(data?: ChallengeInfo): BackendPhase | undefined { | ||
| if (!data) return undefined | ||
|
|
||
| const phases = Array.isArray(data.phases) ? data.phases : [] |
There was a problem hiding this comment.
[correctness]
The selectPhaseForDisplay function assumes that data.phases is always an array if it exists. Consider adding a type guard or validation to ensure data.phases is indeed an array before proceeding, to avoid potential runtime errors.
| return {} | ||
| } | ||
|
|
||
| const endDate = new Date(rawEndDate) |
There was a problem hiding this comment.
[correctness]
The computePhaseTiming function creates a Date object from rawEndDate without checking its format. Ensure rawEndDate is a valid date string or timestamp to prevent NaN errors when creating a Date object.
| } from '../models' | ||
|
|
||
| const resourceBaseUrl = `${EnvironmentConfig.API.V6}` | ||
| const DEFAULT_PER_PAGE = 1000 |
There was a problem hiding this comment.
[performance]
The DEFAULT_PER_PAGE constant is set to 1000, which might be too high depending on the API's performance and the server's capacity. Consider evaluating the optimal number for perPage to balance between performance and data load.
| const firstPage = await xhrGetPaginatedAsync<BackendResource[]>( | ||
| `${resourceBaseUrl}/resources?${qs.stringify(baseQuery)}`, | ||
| ) | ||
| const totalPages = firstPage.totalPages ?? 0 |
There was a problem hiding this comment.
[correctness]
The use of firstPage.totalPages ?? 0 assumes that totalPages is either defined or defaults to 0. Ensure that the API always returns a valid totalPages value to avoid unexpected behavior.
| ) | ||
| } | ||
|
|
||
| const remainingPages = await Promise.all(remainingPagePromises) |
There was a problem hiding this comment.
[performance]
Using Promise.all to fetch all remaining pages concurrently is efficient, but be cautious of potential rate limits or server strain if totalPages is large. Consider implementing a mechanism to handle API rate limits or errors gracefully.
Fix for handles not showing in challenges with many submissions, and a fix for a reported Topgear issue with regards to submission visibility for iterative reviewers. New feature for searching groups in system-admin app