- Notifications
You must be signed in to change notification settings - Fork21
PM-1905 - ai reviews#1277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
PM-1905 - ai reviews#1277
Uh oh!
There was an error while loading.Please reload this page.
Conversation
src/apps/review/src/lib/components/AiReviewsTable/AiReviewsTable.module.scssShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| .workflowName { | ||
| max-width:200px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[💡design]
The.workflowName class usesmax-width: 200px withwhite-space: nowrap,overflow: hidden, andtext-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.scssShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| return( | ||
| <divclassName={styles.wrap}> | ||
| <tableclassName={styles.reviewsTable}> | ||
| <tr> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[readability]
The<tr> elements should be wrapped in a<thead> and<tbody> for better semantic HTML structure and accessibility.
| <> | ||
| <MinusCircleIconclassName='icon icon-xl'/> | ||
| {' '} | ||
| Passed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[❗❗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)){ | ||
| returnrenderSubmissionTab({ | ||
| aiReviewers:( | ||
| challengeInfo?.reviewers?.filter(r=>!!r.aiWorkflowId)as{aiWorkflowId:string}[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[💡maintainability]
The use offilter(r => !!r.aiWorkflowId) assumes thataiWorkflowId is the only property needed fromreviewers. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[correctness]
ThepropertyName 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[❗❗correctness]
Using the non-null assertion operator! onprops.aiReviewers assumes that it is always defined when this code executes. Ifprops.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[💡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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[💡readability]
Using-1 * $sp-4 formargin-left might be less clear than simply using-$sp-4. Consider simplifying the expression for better readability.
| return( | ||
| <divclassName={styles.wrap}> | ||
| <spanclassName={styles.reviewersDropown}onClick={toggleOpen}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[accessibility]
Consider addingrole='button' andtabIndex='0' to the<span> to improve accessibility for keyboard users and screen readers.
| {' '} | ||
| AI Reviewer | ||
| {aiReviewersCount===1 ?'' :'s'} | ||
| <IconOutline.ChevronDownIconclassName='icon-xl'/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[💡usability]
Ensure that theChevronDownIcon rotates when the dropdown is open to provide a visual cue to users.
| } | ||
| } | ||
| .table.aiReviewersTableRow.aiReviewersTableRow { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[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(newSet<string>()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[❗❗correctness]
UsinguseState with aSet can lead to unexpected behavior because React does not detect changes to the internal state of aSet. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[design]
The conditional rendering of the AI Reviewer column header is based on the presence ofprops.aiReviewers. Ifprops.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[maintainability]
ThecolSpan value of 4 assumes a fixed number of columns. If the table structure changes, this could lead to layout issues. Consider dynamically calculatingcolSpan 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[performance]
TheuseSWR hook is using a customfetcher function that makes multiple requests usingPromise.all. This approach could lead to performance issues ifworkflowIds is large, as it will send all requests concurrently. Consider implementing a batching mechanism or limiting the number of concurrent requests to improve performance.
| } | ||
| } | ||
| exportfunctionuseFetchAiWorkflowsRuns( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[💡maintainability]
The function nameuseFetchAiWorkflowsRuns is very similar touseFetchAiWorkflowRuns, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[❗❗correctness]
ChangingisAIReviewer from aboolean toaiWorkflowId as anoptional 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 potentialundefined value. This change could impact logic that previously depended on a boolean check.
src/apps/review/src/lib/components/AiReviewsTable/AiReviewsTable.module.scssShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/apps/review/src/lib/components/AiReviewsTable/AiReviewsTable.module.scssShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/apps/review/src/lib/components/AiReviewsTable/AiReviewsTable.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/apps/review/src/lib/components/ChallengeDetailsContent/TabContentSubmissions.module.scssShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/apps/review/src/lib/components/ChallengeDetailsContent/TabContentSubmissions.tsxShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/apps/review/src/lib/components/ChallengeDetailsContent/TabContentSubmissions.tsxShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/apps/review/src/lib/components/CollapsibleAiReviewsRow/CollapsibleAiReviewsRow.tsxShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/apps/review/src/lib/components/SubmissionHistoryModal/SubmissionHistoryModal.tsxShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/apps/review/src/lib/components/SubmissionHistoryModal/SubmissionHistoryModal.tsxShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
kkartunov left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good
5060ce1 intofeat/ai-workflowsUh oh!
There was an error while loading.Please reload this page.
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:
