[PROD] - AI Workflows MVP #1343
Conversation
…anner PM-2133 dismissable banner
…to feat/ai-workflows
…nges-page PM-1904 active challenges page
…s-page PM-1905 - ai reviews
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| const AiReviewsTable: FC<AiReviewsTableProps> = props => { | ||
| const { runs, isLoading }: AiWorkflowRunsResponse = useFetchAiWorkflowsRuns(props.submission.id) |
There was a problem hiding this comment.
[❗❗ correctness]
The useFetchAiWorkflowsRuns function call no longer includes aiWorkflowIds as a parameter. Ensure that this change is intentional and that the function can operate correctly without this argument. If aiWorkflowIds is required for filtering or other logic within useFetchAiWorkflowsRuns, this could lead to incorrect behavior.
| </span> | ||
| {isOpen && ( | ||
| <div className={classNames(styles.table, 'reviews-table')}> | ||
| <AiReviewsTable submission={props.submission} /> |
There was a problem hiding this comment.
[❗❗ correctness]
The AiReviewsTable component no longer receives the reviewers prop. Ensure that this change is intentional and that AiReviewsTable does not require reviewers for its functionality.
| <tr> | ||
| <td className={styles.aiReviewersTableRow} colSpan={4}> | ||
| <div className={styles.aiReviewersTable}> | ||
| <AiReviewsTable submission={submission} /> |
There was a problem hiding this comment.
[❗❗ correctness]
The AiReviewsTable component no longer receives the reviewers prop. If this prop is necessary for the component's functionality, ensure it is handled appropriately within AiReviewsTable. Otherwise, confirm that this change does not affect the component's behavior.
|
|
||
| export function useFetchAiWorkflowsRuns( | ||
| submissionId: string, | ||
| ): AiWorkflowRunsResponse { |
There was a problem hiding this comment.
[❗❗ correctness]
The workflowIds parameter has been removed, but it is still referenced in the isPaused function. Ensure that this change is intentional and that the logic does not rely on workflowIds being present.
| `${TC_API_BASE_URL}/workflows/runs?submissionId=${submissionId}`, | ||
| { | ||
| isPaused: () => !submissionId, | ||
| refreshInterval: hasInProgress.current ? 10 * 1000 : 0, |
There was a problem hiding this comment.
[performance]
The refreshInterval is set to 10 seconds when hasInProgress.current is true. Consider whether this interval is appropriate for your use case, as frequent polling can lead to performance issues.
| const [actionButtons, setActionButtons] = useState<ReactNode>() | ||
|
|
||
| const challengeDetailsCtx = useContext(ChallengeDetailContext) | ||
| const [submissionInfo] = useFetchSubmissionInfo(submissionId) |
There was a problem hiding this comment.
[❗❗ correctness]
The challengeInfo destructuring has been removed, but aiReviewers and aiWorkflowIds calculations that depend on challengeInfo have also been removed. Ensure that this is intentional and that the logic is not needed elsewhere.
| const challengeDetailsCtx = useContext(ChallengeDetailContext) | ||
| const [submissionInfo] = useFetchSubmissionInfo(submissionId) | ||
|
|
||
| const { runs: workflowRuns, isLoading: aiWorkflowRunsLoading }: AiWorkflowRunsResponse |
There was a problem hiding this comment.
[❗❗ correctness]
The useFetchAiWorkflowsRuns hook is now called without aiWorkflowIds. If this hook relies on these IDs to fetch data correctly, this change might lead to incorrect data fetching. Verify that the hook can function correctly without aiWorkflowIds.
| 'data-tooltip-place': props.place ?? 'bottom', | ||
| 'data-tooltip-strategy': props.strategy ?? 'absolute', | ||
| key: tooltipId.current as string, | ||
| key: `${tooltipId.current as string}-${i}`, |
There was a problem hiding this comment.
[performance]
Using tooltipId.current as part of the key is potentially problematic if tooltipId.current changes over time, as it could lead to unnecessary re-renders. Ensure that tooltipId.current remains constant or consider using a more stable identifier.
Auto refetch run items when workflow run status has changed
| error: fetchError, | ||
| isValidating: isLoading, | ||
| }: SWRResponse<AiWorkflowRunItem[], Error> = useSWR<AiWorkflowRunItem[], Error>( | ||
| `${TC_API_BASE_URL}/workflows/${workflowId}/runs/${runId}/items?[${runStatus}]`, |
There was a problem hiding this comment.
[❗❗ correctness]
The URL construction with runStatus in the query string seems incorrect. Using [${runStatus}] in the URL and then replacing it in the fetcher function is error-prone and could lead to unexpected behavior if runStatus contains special characters. Consider directly appending runStatus to the URL if needed, or ensure proper encoding and validation.
| }: SWRResponse<AiWorkflowRunItem[], Error> = useSWR<AiWorkflowRunItem[], Error>( | ||
| `${TC_API_BASE_URL}/workflows/${workflowId}/runs/${runId}/items?[${runStatus}]`, | ||
| { | ||
| fetcher: url => xhrGetAsync(url.replace(`[${runStatus}]`, '')), |
There was a problem hiding this comment.
[maintainability]
The use of url.replace([${runStatus}], '') in the fetcher function is not ideal. This approach relies on a specific pattern in the URL that could be fragile and lead to bugs if the URL structure changes. Consider constructing the URL correctly from the start or using a more robust method to handle optional query parameters.
| </div> | ||
| </div> | ||
|
|
||
| <div className={styles.modelDescription}> |
There was a problem hiding this comment.
[correctness]
Changing the <p> tag to a <div> tag for modelDescription may affect the semantics and styling of the content. Ensure that the change does not impact the accessibility or intended styling, as <p> tags are typically used for paragraphs of text, which may have default styling and semantic meaning that <div> tags do not provide.
| )} | ||
| </div> | ||
| </div> | ||
| <div className={styles.workflowDescription}> |
There was a problem hiding this comment.
[correctness]
Changing the <p> element to a <div> may affect the semantics and styling of the content. Ensure that this change does not negatively impact accessibility or the intended styling, as <p> elements have default margins that <div> elements do not.
fix(PM-3065, PM-3074, PM-3076): added back button, ui issues
| try { | ||
| if (challengeInfo?.id) { | ||
| // Ensure the challenge details reflect the latest data (e.g., active phase) | ||
| await mutate(`challengeBaseUrl/challenges/${challengeInfo?.id}`) |
There was a problem hiding this comment.
[maintainability]
The mutate function is called with a hardcoded URL string. Consider defining a constant or using a configuration file to manage URLs, which can improve maintainability and reduce the risk of errors if the URL changes.
|
|
||
| const pastPrefix = '/past-challenges/' | ||
| // eslint-disable-next-line no-restricted-globals | ||
| const idx = location.pathname.indexOf(pastPrefix) |
There was a problem hiding this comment.
[correctness]
Using location.pathname directly can lead to issues if the code is executed in a non-browser environment or if the pathname changes. Consider using a more robust method to determine the current path, such as a routing library's API.
| ? `${rootRoute}/past-challenges/${challengeInfo?.id}/challenge-details` | ||
| : `${rootRoute}/active-challenges/${challengeInfo?.id}/challenge-details` | ||
| navigate(url, { | ||
| fallback: './../../../../challenge-details', |
There was a problem hiding this comment.
[correctness]
The fallback URL ./../../../../challenge-details seems relative and could lead to navigation errors if the directory structure changes. Consider using an absolute path or a named route to ensure reliability.
…-renderer Make sure the AI feedback md renderer is not yelling headings
Add ai reviewers on all tables
| :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 are not designed to accommodate this. Consider revisiting this approach to ensure it doesn't cause unintended overlap or spacing issues.
|
|
||
| @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 multiplier can be error-prone and may lead to unexpected layout shifts. Ensure this is intentional and tested across different screen sizes.
| className={styles.aiReviews} | ||
| aiReviewers={props.aiReviewers!} | ||
| submission={submissionPayload} | ||
| defaultOpen={allRows ? !allRows.indexOf(data) : false} |
There was a problem hiding this comment.
[correctness]
The use of ! to assert that props.aiReviewers is not undefined or null can lead to runtime errors if the assumption is incorrect. Consider using optional chaining or a conditional check to ensure safety.
| aiReviewers={props.aiReviewers!} | ||
| submission={submissionPayload} | ||
| defaultOpen={allRows ? !allRows.indexOf(data) : false} | ||
| /> |
There was a problem hiding this comment.
[❗❗ correctness]
The expression !allRows.indexOf(data) will always evaluate to false because indexOf returns -1 for not found and 0 or higher for found indices. Consider using allRows.indexOf(data) === 0 to check if data is the first element.
|
|
||
| @include ltelg { | ||
| width: auto; | ||
| margin-left: -1*$sp-4; |
There was a problem hiding this comment.
[correctness]
The use of -1*$sp-4 for margin-left could be problematic if $sp-4 is not a simple numeric value. Consider using calc() for better clarity and to ensure proper CSS calculation.
| <CollapsibleAiReviewsRow | ||
| className={styles.aiReviews} | ||
| aiReviewers={props.aiReviewers} | ||
| submission={submission as any} |
There was a problem hiding this comment.
[maintainability]
The submission prop is being cast to any. This can lead to runtime errors if the expected structure of submission changes. Consider defining a more specific type for submission to ensure 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 clearer. Consider using allRows.indexOf(submission) === 0 to explicitly check if the submission is the first element, which would be more readable.
| :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 have dynamic content or styles. Consider revisiting this approach to ensure consistent layout behavior.
|
|
||
| @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 multiplier can be error-prone and may lead to layout issues if $sp-4 changes. Consider using a more explicit approach to manage spacing.
| className={styles.aiReviews} | ||
| aiReviewers={props.aiReviewers!} | ||
| submission={submissionPayload as Pick<BackendSubmission, 'id'|'virusScan'>} | ||
| defaultOpen={allRows ? !allRows.indexOf(data) : false} |
There was a problem hiding this comment.
[correctness]
Using the non-null assertion operator (!) on props.aiReviewers could lead to runtime errors if aiReviewers is undefined. Consider adding a check or default value to ensure safety.
| className={styles.aiReviews} | ||
| aiReviewers={props.aiReviewers!} | ||
| submission={submissionPayload as Pick<BackendSubmission, 'id'|'virusScan'>} | ||
| defaultOpen={allRows ? !allRows.indexOf(data) : false} |
There was a problem hiding this comment.
[correctness]
Casting submissionPayload to Pick<BackendSubmission, 'id'|'virusScan'> without verifying all required properties are present could lead to runtime errors. Ensure that submissionPayload contains the necessary properties before casting.
| data={props.screenings} | ||
| data={filteredScreenings} | ||
| showExpand | ||
| expandMode='always' |
There was a problem hiding this comment.
[performance]
The expandMode='always' prop on the Table component may lead to performance issues if the dataset is large, as all rows will be expanded by default. Consider whether this is necessary or if a different approach could be more efficient.
…ables update rendering for ai reviewers
| return ( | ||
| <CollapsibleAiReviewsRow | ||
| className={styles.aiReviews} | ||
| aiReviewers={props.aiReviewers ?? []} |
There was a problem hiding this comment.
[performance]
Using props.aiReviewers ?? [] ensures that aiReviewers is always an array, but it might be more efficient to handle the case where aiReviewers is undefined outside of the useMemo to avoid unnecessary re-renders.
| }, | ||
| { | ||
| ...column, | ||
| colSpan: column.label ? 1 : 2, |
There was a problem hiding this comment.
[💡 readability]
The use of colSpan: column.label ? 1 : 2 is correct, but it might be clearer to explicitly state the logic or extract it into a named constant to improve readability.
| renderer: (submission: SubmissionInfo, allRows: SubmissionInfo[]) => ( | ||
| <CollapsibleAiReviewsRow | ||
| className={styles.aiReviews} | ||
| aiReviewers={props.aiReviewers ?? []} |
There was a problem hiding this comment.
[💡 maintainability]
Using props.aiReviewers ?? [] ensures that aiReviewers is always an array, which is good for preventing runtime errors. However, if aiReviewers is expected to be an array, consider validating this at the prop type level to catch potential issues earlier in the development process.
| const columnsMobile = useMemo<MobileTableColumn<SubmissionInfo>[][]>(() => ( | ||
| (actionColumn ? [...columns, actionColumn] : columns).map(column => ([ | ||
| { | ||
| column.label && { |
There was a problem hiding this comment.
[💡 readability]
The conditional column.label && { ... } could lead to a situation where undefined is pushed into the array, which is why .filter(Boolean) is used later. Consider restructuring this logic to avoid pushing undefined in the first place, which would make the code cleaner and more efficient.
|
|
||
| const aiReviewersColumn = useMemo<TableColumn<Screening> | undefined>( | ||
| () => ({ | ||
| columnId: 'ai-reviews-table', |
There was a problem hiding this comment.
[❗❗ correctness]
The check for props.aiReviewers?.length has been removed. If props.aiReviewers is undefined or an empty array, this could lead to unexpected behavior or errors when accessing props.aiReviewers!. Consider reintroducing the check to ensure props.aiReviewers is defined and not empty before proceeding.
| () => columns.map( | ||
| column => [ | ||
| { | ||
| column.label && { |
There was a problem hiding this comment.
[correctness]
The use of column.label && as a condition could lead to unexpected behavior if column.label is an empty string or a falsy value. Consider explicitly checking for column.label !== undefined to ensure clarity and correctness.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-1428
What's in this PR?
AI Workflows MVP