PM-1944 - ai workflows in reviews/appeals tabs #1330
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const renderSelectedTab = (): JSX.Element => { | ||
| const selectedTabLower = (props.selectedTab || '').toLowerCase() | ||
| const selectedTabNormalized = normalizeType(props.selectedTab) | ||
| const aiReviewers = ( |
There was a problem hiding this comment.
[correctness]
The aiReviewers variable is defined using a type assertion. Consider using a type guard or ensuring the type safety of challengeInfo?.reviewers to avoid potential runtime errors if the structure of reviewers changes.
| :global(.reviews-table) { | ||
| margin-left: auto; | ||
| width: 75%; | ||
| margin-bottom: -9px; |
There was a problem hiding this comment.
[maintainability]
Using a negative margin (margin-bottom: -9px;) can lead to layout issues, especially if the surrounding elements or their styles change. Consider using a different approach to achieve the desired spacing.
|
|
||
| @include ltelg { | ||
| width: auto; | ||
| margin-left: -1*$sp-4; |
There was a problem hiding this comment.
[maintainability]
The use of margin-left: -1*$sp-4; with a negative value can cause unexpected layout shifts, especially in responsive designs. It would be better to review the layout strategy to avoid relying on negative margins.
| isExpand: true, | ||
| label: '', | ||
| renderer: (submission: SubmissionRow, allRows: SubmissionRow[]) => ( | ||
| props.aiReviewers && ( |
There was a problem hiding this comment.
[correctness]
The use of submission as any for type casting can lead to potential runtime errors and makes the code less type-safe. Consider defining a more specific type for submission to maintain type safety.
| className={styles.aiReviews} | ||
| aiReviewers={props.aiReviewers} | ||
| submission={submission as any} | ||
| defaultOpen={allRows ? !allRows.indexOf(submission) : false} |
There was a problem hiding this comment.
[💡 readability]
The expression allRows ? !allRows.indexOf(submission) : false is used to determine the defaultOpen state. This logic could be more explicit. Consider using allRows.indexOf(submission) === 0 to clearly indicate that the first row should be open by default.
| }, | ||
| { | ||
| ...column, | ||
| colSpan: column.label ? 1 : 2, |
There was a problem hiding this comment.
[design]
The colSpan property is conditionally set to 1 or 2 based on column.label. Ensure that this logic aligns with the intended table structure, as incorrect colSpan values can lead to layout issues.
| :global(.reviews-table) { | ||
| margin-left: auto; | ||
| width: 75%; | ||
| margin-bottom: -9px; |
There was a problem hiding this comment.
[maintainability]
Using a negative margin value (-9px) can lead to unexpected layout issues, especially if the surrounding elements or the parent container's layout changes. Consider verifying that this negative margin is necessary and that it does not cause any overlap or visual issues in different screen sizes.
|
|
||
| @include ltelg { | ||
| width: auto; | ||
| margin-left: -1*$sp-4; |
There was a problem hiding this comment.
[maintainability]
The use of -1*$sp-4 for margin-left could lead to maintenance challenges if $sp-4 changes or is used elsewhere with different expectations. Consider defining a specific variable for this negative margin if it's a common pattern, or ensure that its impact is well-understood across different components.
| className={styles.aiReviews} | ||
| aiReviewers={props.aiReviewers} | ||
| submission={submission as any} | ||
| defaultOpen={allRows ? !allRows.indexOf(submission) : false} |
There was a problem hiding this comment.
[correctness]
The use of submission as any for type casting can lead to potential runtime errors and makes the code less type-safe. Consider defining a more specific type for submission to ensure type safety.
| }, | ||
| { | ||
| ...column, | ||
| colSpan: column.label ? 1 : 2, |
There was a problem hiding this comment.
[💡 design]
The colSpan property is conditionally set to 1 or 2 based on the presence of column.label. Ensure that this logic aligns with the intended table layout, as incorrect colSpan values can lead to unexpected rendering issues.
| :global(.reviews-table) { | ||
| margin-left: auto; | ||
| width: 75%; | ||
| margin-bottom: -9px; |
There was a problem hiding this comment.
[maintainability]
Using a negative margin (margin-bottom: -9px;) can lead to unexpected layout issues, especially if the surrounding elements change. Consider using a different layout strategy to achieve the desired spacing.
|
|
||
| @include ltelg { | ||
| width: auto; | ||
| margin-left: -1*$sp-4; |
There was a problem hiding this comment.
[maintainability]
The use of margin-left: -1*$sp-4; with a negative value can cause layout issues and make the code harder to maintain. It would be better to adjust the layout using other CSS properties or parent container styles.
| className={styles.aiReviews} | ||
| aiReviewers={props.aiReviewers} | ||
| submission={submission as any} | ||
| defaultOpen={allRows ? !allRows.indexOf(submission) : false} |
There was a problem hiding this comment.
[correctness]
Casting submission to any can lead to runtime errors if the expected structure changes. Consider using a more specific type or interface to ensure type safety.
|
|
||
| const valueColumn: MobileTableColumn<SubmissionRow> = { | ||
| ...column, | ||
| colSpan: label ? 1 : 2, |
There was a problem hiding this comment.
[💡 maintainability]
The colSpan logic here is duplicated for the valueColumn and the actions column. Consider refactoring to avoid redundancy and potential inconsistencies.
| :global(.reviews-table) { | ||
| margin-left: auto; | ||
| width: 75%; | ||
| margin-bottom: -9px; |
There was a problem hiding this comment.
[maintainability]
The negative margin value -9px for margin-bottom might cause layout issues, especially if the content inside .reviews-table changes or if there are other elements below it. Consider verifying if this is necessary or if it can be adjusted to avoid potential overlap or layout shifts.
|
|
||
| @include ltelg { | ||
| width: auto; | ||
| margin-left: -1*$sp-4; |
There was a problem hiding this comment.
[maintainability]
The use of -1*$sp-4 for margin-left inside the @include ltelg mixin could lead to unexpected layout behavior depending on the value of $sp-4. Ensure that this negative margin is intentional and does not cause content to be misaligned or clipped, especially on larger screens.
| columnId: 'ai-reviews-table', | ||
| isExpand: true, | ||
| label: '', | ||
| renderer: (submission: SubmissionRow, allRows: SubmissionRow[]) => ( |
There was a problem hiding this comment.
[💡 readability]
The renderer function for the ai-reviews-table column uses a conditional check props.aiReviewers && ... which is redundant since this block is already wrapped in a conditional check props.aiReviewers ? .... Consider removing the inner check for clarity.
| const labelText = resolvedLabel || '' | ||
|
|
||
| return [ | ||
| (labelText && ( |
There was a problem hiding this comment.
[💡 readability]
The labelText && ... check is redundant because the labelText is already checked before this line. Consider simplifying the condition to improve readability.
| :global(.reviews-table) { | ||
| margin-left: auto; | ||
| width: 75%; | ||
| margin-bottom: -9px; |
There was a problem hiding this comment.
[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 layout strategy to achieve the desired spacing.
|
|
||
| @include ltelg { | ||
| width: auto; | ||
| margin-left: -1*$sp-4; |
There was a problem hiding this comment.
[maintainability]
The use of margin-left: -1*$sp-4; with a negative value can cause unexpected layout shifts. It's generally better to avoid negative margins unless absolutely necessary, as they can complicate future layout adjustments.
| className={styles.aiReviews} | ||
| aiReviewers={props.aiReviewers} | ||
| submission={submission as any} | ||
| defaultOpen={allRows ? !allRows.indexOf(submission) : false} |
There was a problem hiding this comment.
[maintainability]
The use of submission as any for type casting can lead to potential type safety issues. Consider defining a more specific type for submission to ensure type safety.
| }, | ||
| { | ||
| ...column, | ||
| colSpan: column.label ? 1 : 2, |
There was a problem hiding this comment.
[correctness]
The ternary operation column.label ? 1 : 2 for colSpan could lead to unexpected layout issues if column.label is falsy. Ensure that this logic aligns with the intended design.
| className={styles.aiReviews} | ||
| aiReviewers={props.aiReviewers} | ||
| submission={{ id: result.submissionId, virusScan: true }} | ||
| defaultOpen={allRows ? !allRows.indexOf(result) : false} |
There was a problem hiding this comment.
[💡 readability]
The use of !allRows.indexOf(result) to determine defaultOpen might not be intuitive. Consider using allRows.indexOf(result) === 0 to explicitly check if the result is the first element, which improves readability.
| const columnsMobile = useMemo<MobileTableColumn<ProjectResult>[][]>( | ||
| () => columns.map(column => [ | ||
| { | ||
| column.label && { |
There was a problem hiding this comment.
[💡 readability]
The condition column.label && { ... } creates an object with a potentially falsy value, which is then filtered. This pattern might be confusing. Consider using a more explicit conditional rendering approach to improve clarity.
| readonly moreToLoad?: boolean | ||
| readonly disableSorting?: boolean | ||
| readonly showExpand?: boolean | ||
| readonly expandMode?: 'always' |
There was a problem hiding this comment.
[maintainability]
The expandMode prop is added to the TableProps interface but is not optional. Consider making it optional if it is not required for all table instances.
|
|
||
| .blockExpandItem { | ||
| display: table-row; | ||
| display: flex; |
There was a problem hiding this comment.
[❗❗ correctness]
Changing from display: table-row to display: flex may affect the layout if the parent container is still using display: table. Ensure that the parent container's display property is compatible with flexbox to avoid layout issues.
|
|
||
| .blockExpandCell { | ||
| display: table-cell; | ||
| display: block; |
There was a problem hiding this comment.
[❗❗ correctness]
Switching from display: table-cell to display: block changes the layout behavior significantly. Ensure that this change does not break the intended layout, especially if the parent container or sibling elements rely on table-cell behavior.
| }, [props.columns, props.showExpand]) | ||
| const [isExpanded, setIsExpanded] = useState(false) | ||
| const isAlwaysExpand = props.expandMode === 'always' | ||
| const [isExpanded, setIsExpanded] = useState(isAlwaysExpand) |
There was a problem hiding this comment.
[correctness]
Initializing isExpanded with isAlwaysExpand directly ties the initial expanded state to the expandMode prop. Ensure this is the intended behavior, as it could lead to unexpected UI states if expandMode is changed dynamically.
| index={props.index} | ||
| key={getKey(`${props.index}${colIndex}`)} | ||
| showExpandIndicator={colIndex === 0 && props.showExpand} | ||
| showExpandIndicator={colIndex === 0 && (!isAlwaysExpand && props.showExpand)} |
There was a problem hiding this comment.
[💡 maintainability]
The condition (!isAlwaysExpand && props.showExpand) is used multiple times. Consider extracting this logic into a descriptive variable to improve readability and maintainability.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-1944
https://topcoder.atlassian.net/browse/PM-2134
What's in this PR?
Reviews tab
Appeals
Appeals response
Winners
NOTE: the UI is modified from figma.
The structure of the current UI is too complicated. Refactoring it would take quite a lot of time, and I'm not sure if the purpose of this ticket is refactor/re-structure the UI.
I only integrated the workflow results into current UI.
Also, I'm not sure - should we filter workflows by phase ID? Eg. in screen show only screener ai workflow results. In review show only ai workflows with a reivewer role.
Or for now this is what we want?