fix(PM-3141): Respond to appeals #1366
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| : `/${match[1]}/${match[2]}` | ||
|
|
||
| return `${basePath}/reviews/${submissionId}?reviewId=${encodedReviewId}` | ||
| return `${basePath}/reviews/${submissionId}?reviewId=${encodedReviewId}&respondToAppeals=true` |
There was a problem hiding this comment.
[❗❗ correctness]
Adding &respondToAppeals=true to the query string changes the behavior of the route. Ensure that this change is intentional and that all parts of the application relying on this route are updated accordingly. If this parameter is optional, consider making it conditional to avoid potential issues with existing functionality.
| const ReviewViewer: FC = () => { | ||
| const navigate = useAppNavigate() | ||
| const [searchParams] = useSearchParams() | ||
| const [respondToAppeals, setRespondToAppeals] = useState(searchParams.get("respondToAppeals") === 'true') |
There was a problem hiding this comment.
[correctness]
Using searchParams.get("respondToAppeals") === 'true' directly in the useState initializer can lead to unexpected behavior if the searchParams change after the component mounts. Consider using a useEffect to update respondToAppeals whenever searchParams change.
| setRespondToAppeals(false) | ||
| } | ||
| }, [canEditScorecard, isManagerEdit]) | ||
| }, [canEditScorecard, isManagerEdit, respondToAppeals]) |
There was a problem hiding this comment.
[❗❗ correctness]
The dependency array for this useEffect includes respondToAppeals, which is being set within the effect itself. This can lead to an infinite loop if not handled carefully. Ensure that the logic inside the effect does not inadvertently cause repeated state updates.
| try { | ||
| if (resourceId) { | ||
| // re-fetch appeals for this resourceId | ||
| mutate(`EnvironmentConfig.API.V6/appeals/resourceId/${resourceId}`) |
There was a problem hiding this comment.
[correctness]
The mutate function call has been simplified by removing the second and third arguments. This change means that the cache will not be updated optimistically, and the mutate function will not be called with revalidate set to false. Ensure this change is intentional and that the application logic does not rely on the previous behavior of optimistic updates or preventing revalidation.
| // re-fetch appeals for this resourceId | ||
| mutate(`EnvironmentConfig.API.V6/appeals/resourceId/${resourceId}`) | ||
|
|
||
| } |
There was a problem hiding this comment.
[💡 style]
The removal of trailing whitespace on this line is a minor stylistic change. While it does not affect functionality, ensuring no trailing whitespace can help maintain a clean codebase.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-3141
What's in this PR?