Feat/v6 -> dev #1271
Feat/v6 -> dev #1271
Conversation
fix(PM-1503): QA feedbacks in view scorecard page
…corecard Fix sortOrder for scorecardQuestions & make error more visible
Fixes view and css on Restricted page
…corecard Fix error width
…corecard Fix error width
fix(PM-1503): Weight column in view scorecard page
…corecard PM-1504 - scorecard: Make category options dependent on project type
…corecard PM-1504 - redirect to view scorecard
fix(PM-1503) Used requiresUpload from backend
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ? '' | ||
| : `/${AppSubdomain.review}` | ||
|
|
||
| export const activeReviewAssignmentsRouteId = 'active-challenges' |
There was a problem hiding this comment.
[❗❗ correctness]
The variable name activeReviewAssignmentsRouteId was corrected from activeReviewAssigmentsRouteId. Ensure that all references to this variable in the codebase are updated accordingly to prevent any potential runtime errors.
| import { reviewRoutes } from '../../review-app.routes' | ||
| import { activeReviewAssignmentsRouteId } from '../../config/routes.config' | ||
|
|
||
| export const ActiveReviewAssigments: FC<PropsWithChildren> = () => { |
There was a problem hiding this comment.
[maintainability]
The component name ActiveReviewAssigments contains a typo. It should be ActiveReviewAssignments to match the corrected variable name activeReviewAssignmentsRouteId.
| return childRoutes | ||
| } | ||
|
|
||
| export default ActiveReviewAssigments |
There was a problem hiding this comment.
[maintainability]
The default export ActiveReviewAssigments contains a typo. It should be ActiveReviewAssignments to ensure consistency and avoid confusion.
|
|
||
| const ReviewApp: LazyLoadedComponent = lazyLoad(() => import('./ReviewApp')) | ||
|
|
||
| const ActiveReviewAssigments: LazyLoadedComponent = lazyLoad( |
There was a problem hiding this comment.
[maintainability]
The component ActiveReviewAssigments is imported from a path that contains a typo: active-review-assignements. Consider correcting the directory name to active-review-assignments to maintain consistency and avoid confusion.
| [datas, latestSubmissions, restrictToLatest], | ||
| ) | ||
|
|
||
| if (process.env.NODE_ENV !== 'production') { |
There was a problem hiding this comment.
[security]
The debug logging block is wrapped in a condition checking for process.env.NODE_ENV !== 'production'. Ensure that this condition is correctly set in your deployment environments to avoid leaking potentially sensitive information in non-production environments.
| renderLabel?: () => JSX.Element | ||
| } | ||
| const reviewerColumnMetadata = useMemo<ReviewerColumnMetadata[]>(() => ( | ||
| Array.from({ length: maxReviewCount }, (unused, index) => { |
There was a problem hiding this comment.
[💡 readability]
The use of Array.from with a fixed length and mapping function is correct, but consider using Array(maxReviewCount).fill(null).map(...) for slightly better readability and to avoid the unused variable.
| : resolvedHandle | ||
|
|
||
| const renderLabel = (): JSX.Element => { | ||
| const color = resolvedColor |
There was a problem hiding this comment.
[💡 maintainability]
The inline style for setting the color in renderLabel could be extracted into a CSS class for better maintainability and to separate concerns between styling and logic.
| const reviewId = reviewInfo.id | ||
| const isPendingReopen = pendingReopen?.review?.reviewInfo?.id === reviewId | ||
|
|
||
| function handleReopenClick(): void { |
There was a problem hiding this comment.
[performance]
The handleReopenClick function is defined within the buildReopenAction function. Consider moving it outside to avoid recreating the function on every render, which can improve performance.
| ]) | ||
|
|
||
| const columnsMobile = useMemo<MobileTableColumn<SubmissionRow>[][]>( | ||
| () => columns.map(column => { |
There was a problem hiding this comment.
[performance]
The resolveLabelString function is defined inside the useMemo hook. Consider moving it outside to avoid redefining it on every render, which can improve performance.
|
|
||
| const aggregatedReviews = submission.aggregated?.reviews ?? [] | ||
| const hasIncompleteReview = aggregatedReviews.some(review => { | ||
| const statusRaw = review.reviewInfo?.status ?? review.status |
There was a problem hiding this comment.
[correctness]
The use of review.reviewInfo?.status ?? review.status assumes that review.status is a valid fallback. Ensure that review.status is always defined and meaningful when review.reviewInfo?.status is not available.
| const aggregatedReviews = submission.aggregated?.reviews ?? [] | ||
| const hasIncompleteReview = aggregatedReviews.some(review => { | ||
| const statusRaw = review.reviewInfo?.status ?? review.status | ||
| const normalizedStatus = typeof statusRaw === 'string' |
There was a problem hiding this comment.
[correctness]
The normalization of statusRaw to an empty string when it's not a string type might lead to unexpected behavior. Consider handling non-string types more explicitly to avoid silent failures.
|
|
||
| const reviewerRoleIds = new Set<string>() | ||
| const appendRoleId = (roleId?: string | null): void => { | ||
| if (typeof roleId !== 'string') { |
There was a problem hiding this comment.
[💡 readability]
The function appendRoleId checks if roleId is a string before trimming it. Consider simplifying by using roleId?.trim() directly, which will handle undefined and null gracefully without needing the type check.
| } | ||
|
|
||
| const roleIdMatches = (roleId?: string | null): boolean => { | ||
| if (!roleId) { |
There was a problem hiding this comment.
[💡 readability]
In roleIdMatches, the check if (!roleId) could be simplified by using optional chaining and trimming directly, similar to appendRoleId. This would improve readability and reduce unnecessary checks.
| reviewerResourceIds.forEach(id => fallbackResults.add(id)) | ||
|
|
||
| const shouldRestrictToKnownResources = reviewerResourceIds.size > 0 | ||
| && actionChallengeRole === REVIEWER |
There was a problem hiding this comment.
[💡 readability]
The variable shouldRestrictToKnownResources is used to determine if the resource should be restricted. Consider renaming it to restrictToKnownResources for clarity, as it is a boolean flag.
| const validReviews: BackendSubmission[] = [] | ||
| // Only show CONTEST_SUBMISSION on Review tabs | ||
| forEach(contestSubmissions, challengeSubmission => { | ||
| const normalizedReviewerIds = new Set<string>() |
There was a problem hiding this comment.
[💡 readability]
The function appendReviewerId checks if value is a string before trimming it. Consider simplifying by using value?.trim() directly, which will handle undefined and null gracefully without needing the type check.
| let reviewForResource = matchingReview | ||
|
|
||
| if (assignmentReview) { | ||
| const existingReviewItems = reviewForResource?.reviewItems |
There was a problem hiding this comment.
[💡 readability]
In the assignment of reviewForResource, the spread operator is used twice for reviewForResource and assignmentReview. Consider merging these into a single spread operation to improve readability and reduce redundancy.
|
|
||
| if (!reviewForResource) { | ||
| const emptyReview = { | ||
| ...createEmptyBackendReview(), |
There was a problem hiding this comment.
[design]
The creation of emptyReview uses createEmptyBackendReview() and then assigns resourceId and submissionId. Consider updating createEmptyBackendReview() to accept parameters for these fields to avoid manual assignment.
| const reviewInfo: ReviewInfo | undefined = submission.review | ||
| const reviewId = reviewInfo?.id | ||
| const resourceId = reviewInfo?.resourceId | ||
| const matchingReviewResult = resourceId |
There was a problem hiding this comment.
[maintainability]
The matchingReviewResult variable is declared twice, once at line 138 and again at line 160. The declaration at line 160 is now redundant and can be removed to improve code clarity.
| ? find(submission.reviews, reviewResult => reviewResult.resourceId === resourceId) | ||
| : undefined | ||
| if (!reviewId) { | ||
| if (process.env.NODE_ENV !== 'production') { |
There was a problem hiding this comment.
[💡 design]
The use of process.env.NODE_ENV !== 'production' to conditionally execute debug logging is appropriate for development environments. However, ensure that this pattern is consistently applied across the codebase to avoid accidental logging in production.
| && key.startsWith(`reviewBaseUrl/reviews/${challengeId}/`) | ||
| )), | ||
| mutate(`reviewBaseUrl/submissions/${challengeId}`), | ||
| mutate((key: unknown) => ( |
There was a problem hiding this comment.
[maintainability]
Consider adding a direct mutate call for challengeBaseUrl/submissions/${challengeId} similar to the existing call for reviewBaseUrl/submissions/${challengeId}. This ensures consistency and clarity in cache invalidation logic.
| export * from './default.env' | ||
|
|
||
| export const TC_FINANCE_API = 'http://localhost:3009/v6/finance' | ||
| export const DEBUG_CHECKPOINT_PHASES = true |
There was a problem hiding this comment.
[maintainability]
Setting DEBUG_CHECKPOINT_PHASES to true may inadvertently enable debug logging or behavior in production if this environment file is used beyond local development. Consider ensuring this flag is appropriately managed or documented to prevent unintended side effects.
| [challengeInfo?.phases], | ||
| ) | ||
| const passesReviewTabGuards: (submission: SubmissionInfo) => boolean = useMemo( | ||
| () => (submission: SubmissionInfo): boolean => shouldIncludeInReviewPhase(submission), |
There was a problem hiding this comment.
[❗❗ correctness]
The removal of the disallowedReviewIds and disallowedReviewPhaseIds checks from the passesReviewTabGuards function could lead to incorrect behavior if these checks were intended to filter out certain submissions based on review IDs or phase IDs. Ensure that this change does not affect the correctness of the submission filtering logic.
|
|
||
| const submissionsForAggregation = useMemo<SubmissionInfo[]>( | ||
| () => (restrictToLatest ? latestSubmissions : datas), | ||
| [datas, latestSubmissions, restrictToLatest], |
There was a problem hiding this comment.
[maintainability]
The removal of the debug logging block may hinder debugging in non-production environments. Consider retaining it with a feature flag or a more controlled logging mechanism to aid in development and testing.
| renderLabel?: () => JSX.Element | ||
| } | ||
| const reviewerColumnMetadata = useMemo<ReviewerColumnMetadata[]>(() => ( | ||
| Array.from({ length: maxReviewCount }, (_unused, index) => ({ |
There was a problem hiding this comment.
[❗❗ correctness]
The removal of the logic that dynamically resolves reviewer details, such as handles and ratings, could lead to a loss of important contextual information in the UI. Ensure that this simplification does not negatively impact the user experience or the accuracy of the displayed data.
| return | ||
| } | ||
|
|
||
| registerChallengeReviewKey(challengeId, `reviewBaseUrl/reviews/${challengeId}/${reviewerKey}`) |
There was a problem hiding this comment.
[performance]
The registerChallengeReviewKey function is called inside a useEffect hook, which is correct for side-effects. However, ensure that this function does not have any unintended side-effects or performance implications, especially if the challengeId or reviewerKey change frequently.
| * Record a cache key that should be revalidated when review data changes. | ||
| */ | ||
| export const registerChallengeReviewKey = (challengeId: string | undefined, cacheKey: string | undefined): void => { | ||
| if (!challengeId || !cacheKey) { |
There was a problem hiding this comment.
[maintainability]
Consider throwing an error or logging a warning if challengeId or cacheKey is undefined. Silent failures can make debugging difficult.
| return | ||
| } | ||
|
|
||
| const normalizedKey = cacheKey.trim() |
There was a problem hiding this comment.
[💡 maintainability]
Trimming cacheKey is a good practice, but consider logging or handling the case where cacheKey is only whitespace, as it might indicate an issue elsewhere in the code.
| } | ||
|
|
||
| const registeredReviewKeys = getChallengeReviewKeys(challengeId) | ||
| const reviewListKey = `reviewBaseUrl/reviews?${qs.stringify({ |
There was a problem hiding this comment.
[performance]
The use of perPage: 1000 in the query string for reviewListKey could lead to performance issues if the number of reviews is large. Consider using pagination or a more reasonable limit to avoid potential strain on the server and client.
| perPage: 1000, | ||
| })}` | ||
|
|
||
| const keysToRevalidate = new Set<string>([ |
There was a problem hiding this comment.
[💡 maintainability]
Using new Set<string> to store keysToRevalidate is a good choice for ensuring uniqueness. However, ensure that registeredReviewKeys does not contain duplicates, as this could lead to unnecessary revalidation attempts.
| profileUrl?: string | ||
| } | ||
|
|
||
| function findByResourceId<T extends { resourceId?: string | null }>( |
There was a problem hiding this comment.
[performance]
The function findByResourceId uses items.find(item => item.resourceId === resourceId) which performs a linear search. If items is large or frequently accessed, consider using a more efficient data structure like a Map for better performance.
| return items.find(item => item.resourceId === resourceId) | ||
| } | ||
|
|
||
| function getValueFromMap<T>( |
There was a problem hiding this comment.
[correctness]
The function getValueFromMap accesses the map using map[resourceId]. Ensure that resourceId is a valid key and that the map is correctly populated to avoid potential undefined behavior.
| return undefined | ||
| } | ||
|
|
||
| function buildReviewerDisplayData( |
There was a problem hiding this comment.
[❗❗ correctness]
The function buildReviewerDisplayData has multiple places where it accesses deeply nested properties. Consider adding nullish coalescing or optional chaining to prevent runtime errors if any intermediate property is undefined.
|
|
||
| const resourceId = reviewItem.resourceId | ||
| const submissionId = reviewItem.submissionId | ||
| if (!resourceId || !submissionId) { |
There was a problem hiding this comment.
[❗❗ correctness]
The check for resourceId and submissionId being falsy should be done before using them in the mapping logic. This ensures that the mapping is only updated with valid IDs, preventing potential issues with undefined or null values.
| : undefined | ||
| if (!reviewId) { | ||
| return | ||
| } |
There was a problem hiding this comment.
[❗❗ correctness]
The removal of the check if (seenReviewIds.has(reviewId)) { return } could lead to processing the same review multiple times, which may cause incorrect aggregation results. Consider re-adding this check to ensure each review is processed only once per submission.
| unresolvedAppeals, | ||
| } | ||
|
|
||
| if (existingDetail) { |
There was a problem hiding this comment.
[maintainability]
The logic for updating existingDetail is quite complex and repetitive. Consider refactoring this block into a separate function to improve readability and maintainability.
| const reviewerHandleColorsForGroup: Record<string, string | undefined> = {} | ||
| const reviewerMaxRatingsForGroup: Record<string, number | undefined> = {} | ||
|
|
||
| group.reviews.forEach(review => { |
There was a problem hiding this comment.
[readability]
The forEach loop for resolving reviewer details is complex and involves multiple nested operations. Consider breaking this into smaller functions to enhance readability and reduce cognitive load.
| }, | ||
| { | ||
| resourceId: 'res-2', | ||
| }, |
There was a problem hiding this comment.
[correctness]
The test case assumes that a missing status field implies a pending review. Ensure that resolveSubmissionReviewResult correctly interprets this scenario, as it might lead to incorrect assumptions about the review state.
| { | ||
| finalScore: 84, | ||
| resourceId: 'res-2', | ||
| status: 'SUBMITTED', |
There was a problem hiding this comment.
[correctness]
The status 'SUBMITTED' is used here, but it's unclear if this is considered equivalent to 'COMPLETED' for the purposes of passing the review. Verify that resolveSubmissionReviewResult treats 'SUBMITTED' appropriately, as this could affect the test's validity.
| { | ||
| finalScore: 91, | ||
| resourceId: 'res-1', | ||
| status: 'PENDING', |
There was a problem hiding this comment.
[correctness]
The test case passes a status of 'PENDING' but expects a 'PASS' result. Confirm that resolveSubmissionReviewResult is designed to handle this scenario correctly, as it might lead to unexpected behavior if 'PENDING' is not intended to result in a 'PASS'.
| return normalized.length ? normalized : undefined | ||
| } | ||
|
|
||
| const isAggregatedReviewDetailCompleted = (detail: AggregatedReviewDetail): boolean => { |
There was a problem hiding this comment.
[maintainability]
The function isAggregatedReviewDetailCompleted could be simplified by returning early when a status is found in COMPLETED_REVIEW_STATUSES, rather than continuing to check committedCandidates and finalScoreCandidates. This would improve readability and maintainability.
| return finalScoreCandidates.some(value => typeof value === 'number' && Number.isFinite(value)) | ||
| } | ||
|
|
||
| const shouldDeferAggregatedOutcome = (submission: SubmissionRow): boolean => { |
There was a problem hiding this comment.
[💡 readability]
The function shouldDeferAggregatedOutcome uses multiple Boolean conversions and nullish coalescing which can be simplified. Consider using optional chaining and nullish coalescing directly to improve readability.
| defaultMinimumPassingScore, | ||
| }: ResolveSubmissionReviewResultOptions = options | ||
|
|
||
| if (shouldDeferAggregatedOutcome(submission)) { |
There was a problem hiding this comment.
[❗❗ correctness]
Returning undefined from resolveSubmissionReviewResult when shouldDeferAggregatedOutcome is true might lead to unexpected behavior if the calling code does not handle undefined correctly. Ensure that the calling code is prepared to handle this case.
| }) | ||
| } | ||
|
|
||
| if (!normalizedCandidates.size) { |
There was a problem hiding this comment.
[correctness]
The condition !normalizedCandidates.size is used to determine if the function should return true. This logic might lead to unexpected behavior if no candidates are found, as it defaults to including the submission. Consider revisiting this logic to ensure it aligns with the intended behavior.
|
|
||
| if (process.env.NODE_ENV !== 'production') { | ||
| // eslint-disable-next-line no-console | ||
| console.debug('[ReviewTab] shouldIncludeInReviewPhase', { |
There was a problem hiding this comment.
[💡 security]
Using console.debug for logging in non-production environments is acceptable, but ensure that this logging is necessary and does not expose sensitive information. Consider using a more robust logging mechanism if this is intended for long-term use.
|
|
||
| // Determine if the selected tab corresponds to a phase that hasn't opened yet | ||
| const selectedPhase = useMemo( | ||
| () => (props.selectedPhaseId |
There was a problem hiding this comment.
[performance]
The useMemo hook is used to compute selectedPhase, but it depends on challengeInfo?.phases and props.selectedPhaseId. Ensure that these dependencies are correctly updated to avoid stale values, especially if challengeInfo or selectedPhaseId can change independently.
| const resolvedReviews = useMemo( | ||
| () => { | ||
| if (providedReviews.length) { | ||
| return providedReviews.filter(submission => shouldDisplayOnReviewTab( |
There was a problem hiding this comment.
[performance]
The shouldDisplayOnReviewTab function is called multiple times with the same arguments within the same useMemo block. Consider caching the result to avoid redundant computations, which could improve performance.
| }, | ||
| } | ||
| const converted = convertBackendSubmissionToSubmissionInfo(submissionForReviewer) | ||
| if (shouldDisplayOnReviewTab(converted, challengeInfo?.phases)) { |
There was a problem hiding this comment.
[performance]
The convertBackendSubmissionToSubmissionInfo function is called before checking if the submission should be displayed on the review tab. If this conversion is computationally expensive, consider checking shouldDisplayOnReviewTab first to potentially avoid unnecessary conversions.
| return false | ||
| } | ||
|
|
||
| if (!shouldDisplayOnReviewTab(submission, challengeInfo?.phases)) { |
There was a problem hiding this comment.
[performance]
The shouldDisplayOnReviewTab function is called multiple times with the same arguments within the same useMemo block. Consider caching the result to avoid redundant computations, which could improve performance.
| return fallback | ||
| } | ||
|
|
||
| return providedSubmitterReviews.filter(submission => shouldDisplayOnReviewTab( |
There was a problem hiding this comment.
[performance]
The shouldDisplayOnReviewTab function is called multiple times with the same arguments within the same useMemo block. Consider caching the result to avoid redundant computations, which could improve performance.
| submission: SubmissionInfo, | ||
| phases?: BackendPhase[], | ||
| ): boolean { | ||
| const normalized = new Set<string>() |
There was a problem hiding this comment.
[performance]
The shouldDisplayOnReviewTab function uses a Set to store normalized review types but then converts it to an array for checking exclusions. Consider using the Set directly for exclusions to avoid unnecessary conversion, which could improve performance.
| } | ||
|
|
||
| checkpointDebugEntries.push({ level, namespace, payload }) | ||
| if (checkpointDebugEntries.length > MAX_CHECKPOINT_DEBUG_ENTRIES) { |
There was a problem hiding this comment.
[performance]
The use of checkpointDebugEntries.shift() to remove the oldest entry when the maximum length is exceeded could lead to performance issues if the array grows large. Consider using a more efficient data structure like a queue or a circular buffer.
| } | ||
|
|
||
| const globalTarget = globalThis as Record<string, unknown> | ||
| globalTarget[CHECKPOINT_DEBUG_EXPORT_KEY] = [...checkpointDebugEntries] |
There was a problem hiding this comment.
[security]
Using globalThis to store logs might lead to unintended side effects if other parts of the application or external scripts modify this global object. Consider using a dedicated logging service or module to encapsulate this functionality.
| } | ||
| } | ||
|
|
||
| function applyCanonicalDetails( |
There was a problem hiding this comment.
[correctness]
The function applyCanonicalDetails modifies the normalizedReview object based on preferredPhaseName and preferredTypeId. Ensure that these modifications do not unintentionally overwrite important data from reviewForResource. Consider adding checks or validations before applying these changes.
| return applyCanonicalDetails(baseReview, params.challengeReviewById) | ||
| } | ||
|
|
||
| export interface useFetchScreeningReviewProps { |
There was a problem hiding this comment.
[💡 maintainability]
The interface useFetchScreeningReviewProps is quite extensive. Consider splitting it into smaller interfaces or types for better maintainability and readability.
| reviewItems?: BackendReviewItem[] | ||
| updatedAt: string | ||
| updatedBy: string | ||
| reviewType?: string | null |
There was a problem hiding this comment.
[correctness]
The addition of reviewType?: string | null introduces a potential issue where the type is optional and can be null. Consider whether null is a valid state and if it should be handled explicitly in the code using this interface. If reviewType is critical for certain operations, ensure that the code checks for its presence before use.
| initialScore, | ||
| metadata, | ||
| phaseId: data.phaseId, | ||
| phaseName: typeof data.phaseName === 'string' ? data.phaseName : undefined, |
There was a problem hiding this comment.
[correctness]
The check typeof data.phaseName === 'string' ensures that phaseName is a string before assignment, which is good for type safety. However, consider adding a similar check for data.phaseId to ensure consistency and prevent potential type issues.
| reviewerMaxRating, | ||
| reviewItems: reviewItemsInfo, | ||
| reviewProgress: calculateReviewProgress(reviewItems), | ||
| reviewType: typeof data.reviewType === 'string' ? data.reviewType : undefined, |
There was a problem hiding this comment.
[correctness]
The check typeof data.reviewType === 'string' ensures that reviewType is a string before assignment, which is good for type safety. However, consider adding a similar check for data.status to ensure consistency and prevent potential type issues.
7 participants
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/
What's in this PR?
Moving V6 stuff to dev env to prepare the switch off "-v6" .