Skip to content

update rendering for ai reviewers #1350

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 28, 2025
Merged

update rendering for ai reviewers #1350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12e22a4
update rendering for ai reviewers
vas3a Nov 28, 2025
b2619eb
lint
vas3a Nov 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 27 additions & 32 deletions src/apps/review/src/lib/components/TableCheckpointSubmissions/TableCheckpointSubmissions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,37 +165,31 @@ export const TableCheckpointSubmissions: FC<Props> = (props: Props) => {
)

const aiReviewsColumn = useMemo<TableColumn<Screening> | undefined>(
() => {
if (!props.aiReviewers?.length) {
return undefined
}

return {
columnId: 'ai-reviews-table',
isExpand: true,
label: '',
renderer: (data: Screening, allRows: Screening[]) => {
const submissionPayload = {
id: data.submissionId ?? '',
virusScan: data.virusScan,
} as Pick<BackendSubmission, 'id'|'virusScan'>

if (!submissionPayload.id) {
return <></>
}
() => ({
columnId: 'ai-reviews-table',
isExpand: true,
label: '',
renderer: (data: Screening, allRows: Screening[]) => {
const submissionPayload = {
id: data.submissionId ?? '',
virusScan: data.virusScan,
} as Pick<BackendSubmission, 'id'|'virusScan'>

if (!submissionPayload.id) {
return <></>
}

return (
<CollapsibleAiReviewsRow
className={styles.aiReviews}
aiReviewers={props.aiReviewers!}
submission={submissionPayload}
defaultOpen={allRows ? !allRows.indexOf(data) : false}
/>
)
},
type: 'element',
} as TableColumn<Screening>
},
return (
<CollapsibleAiReviewsRow
className={styles.aiReviews}
aiReviewers={props.aiReviewers ?? []}
submission={submissionPayload}
defaultOpen={allRows ? !allRows.indexOf(data) : false}
/>
)
},
type: 'element',
} as TableColumn<Screening>),
[props.aiReviewers],
)

Expand Down Expand Up @@ -733,7 +727,7 @@ export const TableCheckpointSubmissions: FC<Props> = (props: Props) => {
const columnsMobile = useMemo<MobileTableColumn<Screening>[][]>(
() => columns.map(
column => [
{
column.label && {
Copy link

@github-actions github-actions bot Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 maintainability]
The conditional check column.label && { ... } can lead to a potential issue where undefined is included in the array, which is then filtered out. Consider restructuring this logic to avoid adding undefined in the first place, which can improve readability and maintainability.

...column,
className: '',
label: `${column.label as string} label`,
Expand All @@ -748,9 +742,10 @@ export const TableCheckpointSubmissions: FC<Props> = (props: Props) => {
},
{
...column,
colSpan: column.label ? 1 : 2,
mobileType: 'last-value',
},
] as MobileTableColumn<Screening>[],
].filter(Boolean) as MobileTableColumn<Screening>[],
),
[columns],
)
Expand Down
37 changes: 17 additions & 20 deletions src/apps/review/src/lib/components/TableIterativeReview/TableIterativeReview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1337,24 +1337,20 @@ export const TableIterativeReview: FC<Props> = (props: Props) => {
baseColumns.push(reviewerColumn)
}

if (props.aiReviewers) {
baseColumns.push({
columnId: 'ai-reviews-table',
isExpand: true,
label: '',
renderer: (submission: SubmissionInfo, allRows: SubmissionInfo[]) => (
props.aiReviewers && (
<CollapsibleAiReviewsRow
className={styles.aiReviews}
aiReviewers={props.aiReviewers}
submission={submission as any}
defaultOpen={allRows ? !allRows.indexOf(submission) : false}
/>
)
),
type: 'element',
})
}
baseColumns.push({
columnId: 'ai-reviews-table',
isExpand: true,
label: '',
renderer: (submission: SubmissionInfo, allRows: SubmissionInfo[]) => (
<CollapsibleAiReviewsRow
className={styles.aiReviews}
aiReviewers={props.aiReviewers ?? []}
Copy link

@github-actions github-actions bot Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
Using the nullish coalescing operator (??) to default props.aiReviewers to an empty array is a good practice for avoiding undefined values. However, ensure that the rest of the codebase is prepared to handle an empty array as a valid input, as this might change the behavior if previously undefined was expected to be handled differently.

submission={submission as any}
defaultOpen={allRows ? !allRows.indexOf(submission) : false}
/>
),
type: 'element',
})

return baseColumns
}, [
Expand All @@ -1375,7 +1371,7 @@ export const TableIterativeReview: FC<Props> = (props: Props) => {

const columnsMobile = useMemo<MobileTableColumn<SubmissionInfo>[][]>(() => (
(actionColumn ? [...columns, actionColumn] : columns).map(column => ([
{
column.label && {
Copy link

@github-actions github-actions bot Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
The use of column.label && in the map function could lead to unexpected behavior if column.label is falsy but not undefined or null. Consider explicitly checking for undefined or null if those are the only cases you want to handle.

...column,
className: '',
label: `${column.label as string} label`,
Expand All @@ -1390,9 +1386,10 @@ export const TableIterativeReview: FC<Props> = (props: Props) => {
},
{
...column,
colSpan: column.label ? 1 : 2,
mobileType: 'last-value',
},
] as MobileTableColumn<SubmissionInfo>[]))
].filter(Boolean) as MobileTableColumn<SubmissionInfo>[]))
), [actionColumn, columns])

return (
Expand Down
63 changes: 29 additions & 34 deletions src/apps/review/src/lib/components/TableSubmissionScreening/TableSubmissionScreening.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -789,40 +789,34 @@ export const TableSubmissionScreening: FC<Props> = (props: Props) => {
}, [filteredChallengeSubmissions, props.screenings])

const aiReviewersColumn = useMemo<TableColumn<Screening> | undefined>(
() => {
if (!props.aiReviewers?.length) {
return undefined
}

return {
columnId: 'ai-reviews-table',
isExpand: true,
label: '',
renderer: (
data: Screening,
allRows: Screening[],
) => {
const submissionPayload = submissionMetaById.get(data.submissionId) ?? {
id: data.submissionId ?? '',
virusScan: data.virusScan,
}
() => ({
Copy link

@github-actions github-actions bot Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ performance]
The useMemo hook is used to memoize the aiReviewersColumn even when props.aiReviewers is undefined or empty. This could lead to unnecessary computations and memory usage. Consider adding a condition to return undefined when props.aiReviewers is not present or empty.

columnId: 'ai-reviews-table',
isExpand: true,
label: '',
renderer: (
data: Screening,
allRows: Screening[],
) => {
const submissionPayload = submissionMetaById.get(data.submissionId) ?? {
id: data.submissionId ?? '',
virusScan: data.virusScan,
}

if (!submissionPayload?.id) {
return <></>
}
if (!submissionPayload?.id) {
return <></>
}

return (
<CollapsibleAiReviewsRow
className={styles.aiReviews}
aiReviewers={props.aiReviewers!}
submission={submissionPayload as Pick<BackendSubmission, 'id'|'virusScan'>}
defaultOpen={allRows ? !allRows.indexOf(data) : false}
/>
)
},
type: 'element',
} as TableColumn<Screening>
},
return (
<CollapsibleAiReviewsRow
className={styles.aiReviews}
aiReviewers={props.aiReviewers!}
Copy link

@github-actions github-actions bot Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ correctness]
Using non-null assertion (!) on props.aiReviewers could lead to runtime errors if aiReviewers is undefined. Consider adding a check or providing a default value to ensure safety.

submission={submissionPayload as Pick<BackendSubmission, 'id'|'virusScan'>}
defaultOpen={allRows ? !allRows.indexOf(data) : false}
Copy link

@github-actions github-actions bot Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ correctness]
The expression !allRows.indexOf(data) will always evaluate to false because indexOf returns -1 for non-existent elements, which negates to true. Consider using allRows.indexOf(data) === -1 to check for non-existence.

/>
)
},
type: 'element',
} as TableColumn<Screening>),
[props.aiReviewers, submissionMetaById],
)

Expand Down Expand Up @@ -1191,7 +1185,7 @@ export const TableSubmissionScreening: FC<Props> = (props: Props) => {
const columnsMobile = useMemo<MobileTableColumn<Screening>[][]>(
() => columns.map(
column => [
{
column.label && {
Copy link

@github-actions github-actions bot Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 performance]
The use of column.label && in the map function could lead to a false value being pushed into the array, which is then filtered out. This pattern is not optimal. Consider using a conditional (ternary) operator to directly return the object or null to avoid unnecessary filtering.

...column,
className: '',
label: `${column.label as string} label`,
Expand All @@ -1206,9 +1200,10 @@ export const TableSubmissionScreening: FC<Props> = (props: Props) => {
},
{
...column,
colSpan: column.label ? 1 : 2,
mobileType: 'last-value',
},
] as MobileTableColumn<Screening>[],
].filter(Boolean) as MobileTableColumn<Screening>[],
),
[columns],
)
Expand Down
Loading