fix(PM-3216, PM-2573): dont show edit manager comment to reviewer and filter reviews which are failed screening #1382
Conversation
…ich are failed screening
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| selectedTab={props.selectedTab} | ||
| reviews={reviewTabReviews} | ||
| submitterReviews={reviewTabSubmitterReviews} | ||
| screeningOutcome={screeningOutcome} |
There was a problem hiding this comment.
[correctness]
The screeningOutcome variable is being passed to the TabContentReview component. Ensure that this component is designed to handle the new screeningOutcome prop correctly, especially if it wasn't previously expecting this data. This could impact the component's behavior if not handled properly.
| interface Props { | ||
| aiReviewers?: { aiWorkflowId: string }[] | ||
| selectedTab: string | ||
| screeningOutcome: { |
There was a problem hiding this comment.
[💡 maintainability]
Consider using ReadonlySet<string> for failingSubmissionIds and passingSubmissionIds to ensure immutability and prevent accidental modifications.
| <MarkdownReview value={comment} /> | ||
| </div> | ||
| {isManagerEdit && ( | ||
| {isManagerEdit && canAddManagerComment && ( |
There was a problem hiding this comment.
[correctness]
The addition of canAddManagerComment to the condition ensures that the edit button is only shown when both isManagerEdit and canAddManagerComment are true. Verify that canAddManagerComment is correctly set in all contexts where this component is used, as any oversight could lead to unexpected behavior where the edit button is not displayed when it should be.
| ) | ||
|
|
||
| const filterScreeningPassedReviews = useCallback((submissions: SubmissionInfo[]): SubmissionInfo[] => { | ||
| return submissions.filter(submission => props.screeningOutcome.passingSubmissionIds.has(submission.id ?? '')) |
There was a problem hiding this comment.
[correctness]
Consider adding a null check for submission.id before using it in props.screeningOutcome.passingSubmissionIds.has(submission.id ?? ''). This will prevent potential issues if submission.id is null or undefined.
| () => (restrictToLatest ? latestSubmissions : reviewPhaseDatas), | ||
| [latestSubmissions, restrictToLatest, reviewPhaseDatas], | ||
| () => (restrictToLatest ? filterScreeningPassedReviews(latestSubmissions) : filterScreeningPassedReviews(reviewPhaseDatas)), | ||
| [latestSubmissions, restrictToLatest, reviewPhaseDatas, filterScreeningPassedReviews], |
There was a problem hiding this comment.
[performance]
The dependency array for useMemo includes filterScreeningPassedReviews, which is a function. Functions are recreated on every render, causing this useMemo to recalculate every time. Consider using useCallback for filterScreeningPassedReviews to ensure stable references.
kkartunov
left a comment
kkartunov
left a comment
There was a problem hiding this comment.
@hentrymartin fix lint & build
@kkartunov Lint is fixed now!! |
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-3216
https://topcoder.atlassian.net/browse/PM-2573
What's in this PR?