- Notifications
You must be signed in to change notification settings - Fork21
Add ai reviewers on all tables#1348
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| :global(.reviews-table) { | ||
| margin-left:auto; | ||
| width:75%; | ||
| margin-bottom:-9px; |
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 negative margin value-9px formargin-bottom could lead to unexpected layout issues, especially if the surrounding elements are not designed to accommodate this. Consider verifying the layout impact or using a more consistent spacing strategy.
| @includeltelg { | ||
| width:auto; | ||
| 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.
[maintainability]
Using a negative margin with a multiplication operation-1*$sp-4 can be error-prone if$sp-4 changes or is not well-defined. Ensure that this value is intentional and consistently applied across the codebase.
| className={styles.aiReviews} | ||
| aiReviewers={props.aiReviewers!} | ||
| submission={submissionPayload} | ||
| defaultOpen={allRows ?!allRows.indexOf(data) :false} |
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 use ofallRows.indexOf(data) to determine thedefaultOpen state might not be reliable ifdata is not found inallRows, as it would return-1. Consider using a safer check to determine ifdata is the first element.
| return( | ||
| <CollapsibleAiReviewsRow | ||
| className={styles.aiReviews} | ||
| 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]
The non-null assertionprops.aiReviewers! assumesaiReviewers is always defined whenaiReviewsColumn is used. Ensure thatprops.aiReviewers is validated before this point to avoid potential runtime errors.
| :global(.reviews-table) { | ||
| margin-left:auto; | ||
| width:75%; | ||
| margin-bottom:-9px; |
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]
Using a negative margin (margin-bottom: -9px;) can lead to layout issues, especially if the content or surrounding elements change. Consider using a different approach to achieve the desired layout effect.
| @includeltelg { | ||
| width:auto; | ||
| 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]
The expression-1*$sp-4 formargin-left could be simplified to-$sp-4 for better readability.
| className={styles.aiReviews} | ||
| aiReviewers={props.aiReviewers} | ||
| submission={submissionasany} | ||
| defaultOpen={allRows ?!allRows.indexOf(submission) :false} |
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]
Castingsubmission toany can lead to runtime errors if the expected properties are not present. Consider using a more specific type or interface that matches the expected structure ofsubmission.
| columns={actionColumn ?[...columns,actionColumn] :columns} | ||
| data={datas} | ||
| showExpand | ||
| expandMode='always' |
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]
TheexpandMode='always' property on theTable component will force all rows to be expanded by default. Ensure this is the intended behavior, as it may affect performance and user experience if there are many rows.
| @includeltelg { | ||
| width:auto; | ||
| 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.
[correctness]
The use of-1*$sp-4 formargin-left could be problematic if$sp-4 is not a simple numeric value. Consider usingcalc() for clarity and to ensure proper CSS calculation, e.g.,calc(-1 * #{$sp-4}).
| className={styles.aiReviews} | ||
| aiReviewers={props.aiReviewers!} | ||
| submission={submissionPayloadasPick<BackendSubmission,'id'|'virusScan'>} | ||
| defaultOpen={allRows ?!allRows.indexOf(data) :false} |
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 use of! to assert non-null onprops.aiReviewers could lead to runtime errors ifprops.aiReviewers is unexpectedlyundefined. Consider using optional chaining or a default value to ensure safety.
| className={styles.aiReviews} | ||
| aiReviewers={props.aiReviewers!} | ||
| submission={submissionPayloadasPick<BackendSubmission,'id'|'virusScan'>} | ||
| defaultOpen={allRows ?!allRows.indexOf(data) :false} |
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]
ThedefaultOpen prop calculation using!allRows.indexOf(data) might not work as intended.indexOf returns-1 if the item is not found, which would result intrue fordefaultOpen. Consider usingallRows.indexOf(data) !== -1 for clarity.
f2ab320 intodevUh oh!
There was an error while loading.Please reload this page.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-2993
What's in this PR?
Challenge Review details page - add the ai results for: Screening, Checkpoint Screening, Checkpoint Review, Checkpoint Submission, Iterative Review and Approval tabs