feat(PM-4079): UI updates on Review/Appeals tab#1508
feat(PM-4079): UI updates on Review/Appeals tab#1508hentrymartin wants to merge 8 commits intodevfrom
Conversation
hentrymartin
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| {!runs.length && isLoading && ( | ||
| <tr> | ||
| <td colSpan={4}>Loading...</td> | ||
| <td colSpan={5}>Loading...</td> |
There was a problem hiding this comment.
[maintainability]
The colSpan value was updated from 4 to 5 to accommodate the new 'Min Score' column. Ensure that this change is consistent with all other parts of the code where the table structure is used, especially in different responsive layouts, to avoid layout issues.
| )} | ||
| </td> | ||
| <td> | ||
| {run.workflow?.scorecard?.minimumPassingScore ?? '-'} |
There was a problem hiding this comment.
[correctness]
The use of optional chaining (?.) with run.workflow?.scorecard?.minimumPassingScore is a good practice to prevent runtime errors if any of these properties are undefined. However, ensure that the default value '-' is appropriate for all contexts where this value might be displayed, as it might affect the user interface or data interpretation.
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable complexity */ | |||
There was a problem hiding this comment.
[maintainability]
Disabling ESLint's complexity rule might hide potential maintainability issues. Consider refactoring the component to reduce complexity instead of disabling the rule.
| () => !props.isActiveChallenge && normalizedSelectedTab === 'review', | ||
| [normalizedSelectedTab, props.isActiveChallenge], | ||
| ) | ||
| const isCombinedReviewAppeals = useMemo( |
There was a problem hiding this comment.
[💡 performance]
The useMemo hook is used correctly here to memoize the isCombinedReviewAppeals value. However, ensure that the normalizedSelectedTab value is not expected to change frequently, as excessive use of useMemo can lead to unnecessary complexity.
| downloadSubmission={props.downloadSubmission} | ||
| mappingReviewAppeal={props.mappingReviewAppeal} | ||
| hideHandleColumn={hideHandleColumn} | ||
| mode={isCombinedReviewAppeals ? 'combined-review-appeals' : 'default'} |
There was a problem hiding this comment.
[correctness]
Passing the mode prop with a dynamic value is a good approach. Ensure that the TableReview component handles both 'combined-review-appeals' and 'default' modes correctly, especially if there are significant differences in behavior.
|
|
||
| &:global(.enhanced-table) table tbody tr:global(.row-before-expand) td[data-col-index='0'], | ||
| &:global(.enhanced-table) table tbody tr:global(.row-before-expand) td[data-col-index='1'] { | ||
| border-bottom: none !important; |
There was a problem hiding this comment.
[maintainability]
Using !important can make future maintenance difficult as it overrides other styles. Consider if !important is necessary here or if the specificity of the selector can be increased instead.
| } | ||
|
|
||
| &:global(.enhanced-table) table tbody tr:not(:global(.expand-row)) td { | ||
| border-top: none !important; |
There was a problem hiding this comment.
[maintainability]
Using !important can make future maintenance difficult as it overrides other styles. Consider if !important is necessary here or if the specificity of the selector can be increased instead.
| } | ||
|
|
||
| &:global(.enhanced-table) table tbody tr:global(.expand-row):has(.aiReviews) td { | ||
| border-bottom: var(--TableBorderColor) solid 1px !important; |
There was a problem hiding this comment.
[maintainability]
Using !important can make future maintenance difficult as it overrides other styles. Consider if !important is necessary here or if the specificity of the selector can be increased instead.
| for (const aggregated of list) { | ||
| const base = { | ||
| ...(aggregated.submission ?? {}), | ||
| ...aggregated.submission, |
There was a problem hiding this comment.
[correctness]
The spread operator is used twice on aggregated.submission. This is redundant and could lead to unexpected behavior if properties are overwritten. Consider using it only once.
| ]) | ||
|
|
||
| const tableRows = useMemo<SubmissionRow[]>( | ||
| () => (props.mode === 'combined-review-appeals' ? expandedReviewRows : aggregatedRows), |
There was a problem hiding this comment.
[❗❗ correctness]
The useMemo dependency array includes aggregatedRows, which is not defined in the diff. Ensure that aggregatedRows is correctly defined and included in the dependencies.
| } | ||
|
|
||
| // Combined Review/Appeals layout | ||
| if (props.mode === 'combined-review-appeals') { |
There was a problem hiding this comment.
[💡 performance]
The submissionGroupRowSpan function iterates over allRows to count rows with the same submissionId. This could be optimized by using a more efficient data structure or algorithm if performance becomes an issue with large datasets.
| } | ||
|
|
||
| if (props.aiReviewers) { | ||
| combinedColumns.push({ |
There was a problem hiding this comment.
[design]
The combinedColumns array is conditionally extended with AI review columns. Ensure that props.aiReviewers is always defined when expected, as this could lead to inconsistent table structures.
| disableSorting | ||
| onToggleSort={_.noop} | ||
| removeDefaultSort | ||
| expandContentStartIndex={ |
There was a problem hiding this comment.
[correctness]
The expandContentStartIndex is conditionally set based on props.mode. Ensure that the index calculation is correct and that columns always contains the expected structure to avoid runtime errors.
| const reviewTab = tabs.find(tab => { | ||
| const normalizedValue = normalize(tab.value) | ||
| return normalizedValue === 'review' | ||
| || normalizedValue === 'review / appeals' |
There was a problem hiding this comment.
[maintainability]
Consider using a more robust method for comparing tab values, such as a set of allowed values, to avoid potential issues with string comparison errors or typos. This will improve maintainability and reduce the risk of bugs if more tab names are added in the future.
...ps/review/src/pages/active-review-assignements/ChallengeDetailsPage/ChallengeDetailsPage.tsx
Show resolved
Hide resolved
...ps/review/src/pages/active-review-assignements/ChallengeDetailsPage/ChallengeDetailsPage.tsx
Show resolved
Hide resolved
...ps/review/src/pages/active-review-assignements/ChallengeDetailsPage/ChallengeDetailsPage.tsx
Show resolved
Hide resolved
...ps/review/src/pages/active-review-assignements/ChallengeDetailsPage/ChallengeDetailsPage.tsx
Show resolved
Hide resolved
...ps/review/src/pages/active-review-assignements/ChallengeDetailsPage/ChallengeDetailsPage.tsx
Show resolved
Hide resolved
| {!!col.label && ( | ||
| <strong | ||
| {props.showExpand && isExpanded && (() => { | ||
| const skippedByRowSpan = props.skipCellByColumn?.filter(Boolean).length ?? 0 |
There was a problem hiding this comment.
[correctness]
The calculation of effectiveSkippedCount assumes that skipCellByColumn and expandContentStartIndex are always defined and valid. Consider adding validation or default values to handle cases where these props might be undefined or contain unexpected values.
| // eslint-disable-next-line jsx-a11y/control-has-associated-label | ||
| <td key={`expand-pad-${i}`} /> | ||
| ))} | ||
| <td className={styles.expandContentCell} colSpan={expandColSpan}> |
There was a problem hiding this comment.
[❗❗ correctness]
The colSpan attribute for the <td> element is calculated as displayColumns.length - effectiveSkippedCount. Ensure that effectiveSkippedCount does not exceed displayColumns.length to prevent rendering issues.
| indexColumns | ||
| === props.columns.length - 1 | ||
| (itemItemColumns, indexItemColumns) => { | ||
| const { rowSpan: rowSpanOrFn, ...cellColumnProps } = itemItemColumns |
There was a problem hiding this comment.
[performance]
The use of rowSpanOrFn as a function to determine rowSpan is flexible, but ensure that the function is performant and doesn't introduce unnecessary complexity or performance overhead, especially if props.data is large or complex.
kkartunov
left a comment
kkartunov
left a comment
There was a problem hiding this comment.
@hentrymartin remove the combo - we said we want to keep phase tabs separated.
2 participants
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-4079
What's in this PR?