[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}
| 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.
fix(PM-3090): likes dislikes not updated automatically
| await createFeedbackComment(workflowId as string, workflowRun?.id as string, feedback?.id, { | ||
| content, | ||
| }) | ||
| await mutate(` |
There was a problem hiding this comment.
[💡 readability]
The use of template literals for the mutate function call introduces unnecessary complexity and potential for errors. Consider simplifying the URL construction by removing the template literal and constructing the URL string directly.
|
|
||
| try { | ||
| // eslint-disable-next-line max-len | ||
| const itemsKey = `${EnvironmentConfig.API.V6}/workflows/${workflowId}/runs/${workflowRun.id}/items?[${workflowRun?.status}]` |
There was a problem hiding this comment.
[❗❗ correctness]
Appending ?[${workflowRun?.status}] to the URL might lead to unexpected behavior if workflowRun?.status is undefined or null. Consider validating workflowRun?.status before appending it to ensure the URL is constructed correctly.
| setDownVotes(prevDown) | ||
|
|
||
| // eslint-disable-next-line max-len | ||
| const itemsKey = `${EnvironmentConfig.API.V6}/workflows/${workflowId}/runs/${workflowRun.id}/items?[${workflowRun?.status}]` |
There was a problem hiding this comment.
[❗❗ correctness]
The URL construction with ?[${workflowRun?.status}] should be validated to ensure workflowRun?.status is not undefined or null, which could result in malformed URLs.
| upVote: up, | ||
| }) | ||
| // eslint-disable-next-line max-len | ||
| await mutate(`${EnvironmentConfig.API.V6}/workflows/${workflowId}/runs/${workflowRun?.id}/items?[${workflowRun?.status}]`) |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure workflowRun?.status is validated before appending to the URL to prevent potential issues with malformed URLs if workflowRun?.status is undefined or null.
| parentId: comment.id, | ||
| }) | ||
| await mutate(` | ||
| ${EnvironmentConfig.API.V6}/workflows/${workflowId}/runs/${workflowRun?.id}/items?[${workflowRun?.status}] |
There was a problem hiding this comment.
[❗❗ correctness]
The use of a template literal with a dynamic query parameter [${workflowRun?.status}] in the mutate function call is unconventional and could lead to unexpected behavior. Ensure that the API endpoint supports this query parameter format and that it is necessary for the intended functionality.
| content, | ||
| }) | ||
| await mutate(` | ||
| ${EnvironmentConfig.API.V6}/workflows/${workflowId}/runs/${workflowRun?.id}/items?[${workflowRun?.status}] |
There was a problem hiding this comment.
[❗❗ correctness]
The use of a template literal with a dynamic query parameter [${workflowRun?.status}] in the mutate function call is unconventional and could lead to unexpected behavior. Ensure that the API endpoint supports this query parameter format and that it is necessary for the intended functionality.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-1428
What's in this PR?
AI Workflows MVP