PM-1905 - ai reviews #1277
PM-1905 - ai reviews #1277
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/apps/review/src/lib/components/AiReviewsTable/AiReviewsTable.module.scss
Show resolved
Hide resolved
| } | ||
|
|
||
| .workflowName { | ||
| max-width: 200px; |
There was a problem hiding this comment.
[💡 design]
The .workflowName class uses max-width: 200px with white-space: nowrap, overflow: hidden, and text-overflow: ellipsis. This combination will truncate text that exceeds 200px, which might not be suitable for all content. Consider whether this truncation is acceptable for all cases.
src/apps/review/src/lib/components/AiReviewsTable/AiReviewsTable.module.scss
Show resolved
Hide resolved
| return ( | ||
| <div className={styles.wrap}> | ||
| <table className={styles.reviewsTable}> | ||
| <tr> |
There was a problem hiding this comment.
[readability]
The <tr> elements should be wrapped in a <thead> and <tbody> for better semantic HTML structure and accessibility.
| <> | ||
| <MinusCircleIcon className='icon icon-xl' /> | ||
| {' '} | ||
| Passed |
There was a problem hiding this comment.
[❗❗ correctness]
The text 'Passed' is used in both branches of the conditional rendering. It seems like a mistake since the icon suggests a different result. Consider changing the text in the else branch to 'Failed' or another appropriate term.
| if (SUBMISSION_TAB_KEYS.has(selectedTabNormalized)) { | ||
| return renderSubmissionTab({ | ||
| aiReviewers: ( | ||
| challengeInfo?.reviewers?.filter(r => !!r.aiWorkflowId) as { aiWorkflowId: string }[] |
There was a problem hiding this comment.
[💡 maintainability]
The use of filter(r => !!r.aiWorkflowId) assumes that aiWorkflowId is the only property needed from reviewers. If additional properties are required in the future, this filtering logic might need to be updated. Consider whether this assumption is safe for future changes.
| }, | ||
| type: 'element', | ||
| }, | ||
| ...(!props.aiReviewers?.length ? [] : [{ |
There was a problem hiding this comment.
[correctness]
The propertyName for the 'Reviewer' column is set to 'submittedDate', which seems incorrect. This could lead to confusion or errors when accessing the column data. Consider using a more appropriate property name that reflects the column's purpose.
| propertyName: 'submittedDate', | ||
| renderer: (submission: BackendSubmission) => ( | ||
| <CollapsibleAiReviewsRow | ||
| aiReviewers={props.aiReviewers!} |
There was a problem hiding this comment.
[❗❗ correctness]
Using the non-null assertion operator ! on props.aiReviewers assumes that it is always defined when this code executes. If props.aiReviewers can be undefined, this could lead to runtime errors. Consider adding a check or default value to ensure safety.
| text-align: left; | ||
| } | ||
|
|
||
| .reviewersDropown { |
There was a problem hiding this comment.
[💡 readability]
The class name .reviewersDropown appears to have a typo. It should likely be .reviewersDropdown for clarity and consistency.
|
|
||
| .table { | ||
| margin-top: $sp-2; | ||
| margin-left: -1 * $sp-4; |
There was a problem hiding this comment.
[💡 readability]
Using -1 * $sp-4 for margin-left might be less clear than simply using -$sp-4. Consider simplifying the expression for better readability.
|
|
||
| return ( | ||
| <div className={styles.wrap}> | ||
| <span className={styles.reviewersDropown} onClick={toggleOpen}> |
There was a problem hiding this comment.
[accessibility]
Consider adding role='button' and tabIndex='0' to the <span> to improve accessibility for keyboard users and screen readers.
| {' '} | ||
| AI Reviewer | ||
| {aiReviewersCount === 1 ? '' : 's'} | ||
| <IconOutline.ChevronDownIcon className='icon-xl' /> |
There was a problem hiding this comment.
[💡 usability]
Ensure that the ChevronDownIcon rotates when the dropdown is open to provide a visual cue to users.
| } | ||
| } | ||
|
|
||
| .table .aiReviewersTableRow.aiReviewersTableRow { |
There was a problem hiding this comment.
[maintainability]
The selector .table .aiReviewersTableRow.aiReviewersTableRow seems to have a duplicated class name. This could lead to confusion or unintended styling issues. Consider removing the duplicate class name.
| [props.submissions], | ||
| ) | ||
|
|
||
| const [toggledRows, setToggledRows] = useState(new Set<string>()) |
There was a problem hiding this comment.
[❗❗ correctness]
Using useState with a Set can lead to unexpected behavior because React does not detect changes to the internal state of a Set. Consider using an array or another data structure that React can track changes to more effectively.
| <span>-</span> | ||
| )} | ||
| </td> | ||
| {!!props.aiReviewers?.length && ( |
There was a problem hiding this comment.
[design]
The conditional rendering of the AI Reviewer column header is based on the presence of props.aiReviewers. If props.aiReviewers is undefined or an empty array, this column will not be rendered, which might lead to inconsistent table structures. Ensure that the table structure remains consistent regardless of the data.
| </td> | ||
| </tr> | ||
| </tr> | ||
| {toggledRows.has(submission.id) && ( |
There was a problem hiding this comment.
[maintainability]
The colSpan value of 4 assumes a fixed number of columns. If the table structure changes, this could lead to layout issues. Consider dynamically calculating colSpan based on the number of columns to ensure future maintainability.
| error: fetchError, | ||
| isValidating: isLoading, | ||
| }: SWRResponse<AiWorkflowRun[], Error> = useSWR<AiWorkflowRun[], Error>( | ||
| `${TC_API_BASE_URL}/workflows/${workflowIds.join(',')}/runs?submissionId=${submissionId}`, |
There was a problem hiding this comment.
[performance]
The useSWR hook is using a custom fetcher function that makes multiple requests using Promise.all. This approach could lead to performance issues if workflowIds is large, as it will send all requests concurrently. Consider implementing a batching mechanism or limiting the number of concurrent requests to improve performance.
| } | ||
| } | ||
|
|
||
| export function useFetchAiWorkflowsRuns( |
There was a problem hiding this comment.
[💡 maintainability]
The function name useFetchAiWorkflowsRuns is very similar to useFetchAiWorkflowRuns, which could lead to confusion. Consider renaming one of the functions to more clearly distinguish their purposes.
| incrementalPayment: number | ||
| type: string | ||
| isAIReviewer: boolean | ||
| aiWorkflowId?: string; |
There was a problem hiding this comment.
[❗❗ correctness]
Changing isAIReviewer from a boolean to aiWorkflowId as an optional string alters the data model significantly. Ensure that all parts of the codebase that rely on this field are updated accordingly to handle the new type and potential undefined value. This change could impact logic that previously depended on a boolean check.
src/apps/review/src/lib/components/AiReviewsTable/AiReviewsTable.module.scss
Show resolved
Hide resolved
src/apps/review/src/lib/components/AiReviewsTable/AiReviewsTable.module.scss
Show resolved
Hide resolved
src/apps/review/src/lib/components/AiReviewsTable/AiReviewsTable.tsx
Outdated
Show resolved
Hide resolved
src/apps/review/src/lib/components/ChallengeDetailsContent/TabContentSubmissions.module.scss
Show resolved
Hide resolved
src/apps/review/src/lib/components/ChallengeDetailsContent/TabContentSubmissions.tsx
Show resolved
Hide resolved
src/apps/review/src/lib/components/ChallengeDetailsContent/TabContentSubmissions.tsx
Show resolved
Hide resolved
src/apps/review/src/lib/components/CollapsibleAiReviewsRow/CollapsibleAiReviewsRow.tsx
Show resolved
Hide resolved
src/apps/review/src/lib/components/SubmissionHistoryModal/SubmissionHistoryModal.tsx
Show resolved
Hide resolved
src/apps/review/src/lib/components/SubmissionHistoryModal/SubmissionHistoryModal.tsx
Show resolved
Hide resolved
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-1905
What's in this PR?
Note: reviews details will be fetched only on row expand:
