[PROD] - AI Workflows MVP #7151
Conversation
PM- 3026 Update Manage Submissions to accommodate AI workflows
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| className="src-shared-components-SubmissionManagement-SubmissionsTable-___styles__workflow-table___WCWMZ" | ||
| > | ||
| <TableWorkflowRuns | ||
| workflowRuns={null} |
There was a problem hiding this comment.
[❗❗ correctness]
Passing null to workflowRuns might lead to unexpected behavior if TableWorkflowRuns does not handle null values gracefully. Consider ensuring that TableWorkflowRuns can handle null or provide a default value.
| * object should be considered outdated, and updated as soon as possible. */ | ||
| USER_GROUP_MAXAGE: 24 * 60 * 60 * 1000, | ||
|
|
||
| REVIEW_APP_URL: 'https://review.topcoder-dev.com', |
There was a problem hiding this comment.
[maintainability]
The REVIEW_APP_URL is hardcoded to a specific environment (topcoder-dev). If this configuration is intended to be used across different environments, consider parameterizing this URL to avoid potential issues when deploying to production or other environments.
| * This value [seconds] specifies the maximum age after which a group data | ||
| * object should be considered outdated, and updated as soon as possible. */ | ||
| USER_GROUP_MAXAGE: 24 * 60 * 60 * 1000, | ||
| REVIEW_APP_URL: 'https://review.topcoder-dev.com', |
There was a problem hiding this comment.
[💡 maintainability]
Consider adding a comment or documentation for REVIEW_APP_URL to clarify its purpose and usage, especially if it is intended for a specific environment or use case. This will help maintainability by ensuring future developers understand its role.
|
|
||
| function loadAiWorkflowRunsDone(tokenV3, submissionId, aiWorkflowId) { | ||
| const api = new Api(config.API.V6, tokenV3); | ||
| const url = `/workflows/${aiWorkflowId}/runs?submissionId=${submissionId}`; |
There was a problem hiding this comment.
[security]
Consider using encodeURIComponent on submissionId and aiWorkflowId to prevent potential issues with special characters in the URL.
| aiWorkflowId, | ||
| runs: data, | ||
| })) | ||
| .catch((err) => { |
There was a problem hiding this comment.
[💡 maintainability]
Catching the error and re-throwing it without additional handling or logging may not be useful. Consider logging the error or providing more context before re-throwing.
| title: '', | ||
| classname: '', | ||
| message: 'Your submission has been received, and will be evaluated during Review phase.', | ||
| message: 'Your submission has been received and may undergo AI-assisted review during Submission phase. Results will be available for inspection in the review app and final evaluation occurs during Review phase.', |
There was a problem hiding this comment.
[💡 readability]
The updated message is longer and more complex, which could impact readability. Consider breaking it into shorter sentences or simplifying the language to improve clarity.
| } | ||
| } | ||
|
|
||
| console.log('showScreeningDetails updated to:', showScreeningDetails); |
There was a problem hiding this comment.
[❗❗ maintainability]
Remove the console.log statement. Debugging logs should not be present in production code as they can clutter the console and potentially expose sensitive information.
| const onOpenRatingsList = onOpenRatingsListModal.bind(1, submissionObject.id); | ||
| const onOpenReviewApp = () => { | ||
| if (!challenge || !challenge.id) return; | ||
| const url = `${config.REVIEW_APP_URL}/active-challenges/${challenge.id}/challenge-details?tab=submission`; |
There was a problem hiding this comment.
[correctness]
Consider validating config.REVIEW_APP_URL to ensure it is defined and a valid URL before constructing the url variable. This will prevent potential runtime errors if config.REVIEW_APP_URL is undefined or malformed.
| .review-button { | ||
| cursor: pointer; | ||
| color: $color-turq-160; | ||
| font-size: small; |
There was a problem hiding this comment.
[💡 maintainability]
Consider using a specific font size value instead of small for consistency and better control over the appearance across different browsers.
| SubmissionManagement.propTypes = { | ||
| challenge: PT.shape().isRequired, | ||
| showDetails: PT.shape().isRequired, | ||
| submissionWorkflowRuns: PT.shape().isRequired, |
There was a problem hiding this comment.
[maintainability]
The submissionWorkflowRuns prop is defined as PT.shape().isRequired, but the shape is not specified. Consider defining the expected shape for better type safety and to prevent runtime errors.
|
|
||
| SubmissionsTable.propTypes = { | ||
| challenge: PT.shape().isRequired, | ||
| submissionWorkflowRuns: PT.shape(), |
There was a problem hiding this comment.
[❗❗ correctness]
The submissionWorkflowRuns prop is defined as a PT.shape(), which implies an object, but its default value is set to an empty array in defaultProps. This mismatch can lead to unexpected behavior. Consider setting the default value to an empty object {} to match the prop type.
|
|
||
| export default function TableWorkflowRuns(props) { | ||
| const { workflowRuns } = props; | ||
| if (!workflowRuns || Object.keys(workflowRuns).length === 0) { |
There was a problem hiding this comment.
[performance]
Using Object.keys(workflowRuns).length === 0 to check for an empty object is less efficient than Object.entries(workflowRuns).length === 0, which is already used in the map function. Consider using Object.entries(workflowRuns).length === 0 for consistency and potential performance improvement.
| }; | ||
|
|
||
| TableWorkflowRuns.propTypes = { | ||
| workflowRuns: PT.shape(), |
There was a problem hiding this comment.
[maintainability]
The workflowRuns prop type is defined as PT.shape(), which does not specify the expected structure of the workflowRuns object. This can lead to runtime errors if the object does not have the expected shape. Consider defining a more specific shape for workflowRuns to improve type safety.
| font-weight: 600; | ||
| line-height: $status-space-20; | ||
| vertical-align: middle; | ||
| padding: 8px 12px !important; |
There was a problem hiding this comment.
[maintainability]
Avoid using !important unless absolutely necessary, as it can make the CSS harder to maintain and override in the future. Consider refactoring to achieve the desired styling without !important.
| } = this.props; | ||
| const { initialState } = this.state; | ||
|
|
||
| if (!challenge || !Array.isArray(challenge.reviewers) || !mySubmissions) return; |
There was a problem hiding this comment.
[maintainability]
The check !challenge || !Array.isArray(challenge.reviewers) || !mySubmissions is used to return early. However, this could lead to silent failures if any of these conditions are unexpectedly true. Consider logging a warning or error to help with debugging.
| return; | ||
| } | ||
|
|
||
| if (mySubmissions.length && !this.firstSubmissionExpanded) { |
There was a problem hiding this comment.
[correctness]
The condition mySubmissions.length && !this.firstSubmissionExpanded could lead to unexpected behavior if mySubmissions is modified asynchronously. Consider using a more robust state management approach to ensure consistency.
| dispatch(a.getSubmissionsDone(challengeId, tokens.tokenV3)); | ||
| }, | ||
|
|
||
| loadAiWorkflowRuns: (tokens, submissionId, aiWorkflowId) => { |
There was a problem hiding this comment.
[correctness]
The loadAiWorkflowRuns function dispatches two actions sequentially. If these actions are dependent on each other, consider using a promise chain or async/await to ensure the correct order of execution.
| toBeDeletedId: payload, | ||
| deletionSucceed: true, | ||
| }), | ||
| [a.loadAiWorkflowRunsInit]: state => ({ ...state, isLoadingDetails: true }), |
There was a problem hiding this comment.
[💡 maintainability]
Consider initializing isLoadingDetails in the default state to avoid potential undefined state issues. This can improve the robustness of the state management.
|
|
||
| function loadAiWorkflowRunsDone(tokenV3, submissionId) { | ||
| const api = new Api(config.API.V6, tokenV3); | ||
| const url = `/workflows/runs?submissionId=${submissionId}`; |
There was a problem hiding this comment.
[❗❗ correctness]
The removal of aiWorkflowId from the URL path changes the endpoint being called. Ensure that this change is intentional and that the endpoint /workflows/runs is correct and expected to handle requests without the aiWorkflowId parameter.
| .then(res => res.json()) | ||
| .then(data => ({ | ||
| submissionId, | ||
| runs: data, |
There was a problem hiding this comment.
[❗❗ correctness]
The aiWorkflowId property is removed from the returned object. Verify that this change does not affect any consumers of this function that might rely on aiWorkflowId being present in the response.
| const getRunStatusText = (run) => { | ||
| if (!run) return ''; | ||
|
|
||
| if (run.status === 'IN_PROGRESS' || run.status === 'QUEUED') return 'PENDING'; |
There was a problem hiding this comment.
[maintainability]
Consider using a consistent casing for status strings throughout the application. If the status strings are used elsewhere in lowercase, this change might lead to inconsistencies unless all usages are updated.
| }; | ||
|
|
||
| export default function TableWorkflowRuns(props) { | ||
| const { workflowRuns, challengeId } = props; |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that challengeId is always provided and valid when TableWorkflowRuns is used, as it is now part of the URL construction. This could lead to broken links if challengeId is missing or incorrect.
| }; | ||
|
|
||
| TableWorkflowRuns.propTypes = { | ||
| workflowRuns: PT.shape(), |
There was a problem hiding this comment.
[maintainability]
The workflowRuns prop type should be more specific than PT.shape(). Consider defining the expected shape of the workflowRuns object to improve type safety and maintainability.
|
|
||
| mySubmissions.forEach((submission) => { | ||
| const key = `${submission.id}`; | ||
| if (this.loadedWorkflowKeys.has(key)) return; |
There was a problem hiding this comment.
[❗❗ correctness]
The key for loadedWorkflowKeys has been changed from ${submission.id}-${reviewer.aiWorkflowId} to just ${submission.id}. This change assumes that submission.id is unique across all workflows, which may not be the case if multiple workflows are associated with the same submission. Consider whether this change could lead to incorrect behavior if multiple workflows are expected per submission.
| dispatch(a.getSubmissionsDone(challengeId, tokens.tokenV3)); | ||
| }, | ||
|
|
||
| loadAiWorkflowRuns: (tokens, submissionId) => { |
There was a problem hiding this comment.
[❗❗ correctness]
The loadAiWorkflowRuns function signature has been changed to remove aiWorkflowId. Ensure that this change aligns with the intended logic, as it assumes that aiWorkflowId is no longer needed. If aiWorkflowId is required for identifying specific workflow runs, this change could lead to incorrect behavior.
PM-3026 Update tab for checkpoint submissions
PM-3026 Fix QA Feedback
| const onOpenRatingsList = onOpenRatingsListModal.bind(1, submissionObject.id); | ||
| const onOpenReviewApp = () => { | ||
| if (!challenge || !challenge.id) return; | ||
| const tab = submissionObject.type === 'CHECKPOINT_SUBMISSION' |
There was a problem hiding this comment.
[❗❗ correctness]
The removal of the condition submissionObject.type === 'CONTEST_SUBMISSION' may affect the logic for determining the tab value. Ensure that this change is intentional and that CONTEST_SUBMISSION should no longer be treated as a 'checkpoint-submission'.
| {Object.entries(workflowRuns).map(([workflowId, run]) => ( | ||
| <tr key={workflowId}> | ||
| <td>{run.workflow.name}</td> | ||
| <td>{run.status === 'SUCCESS' ? ( |
There was a problem hiding this comment.
[correctness]
Consider using a more explicit check for run.completedAt before formatting it with moment. If run.completedAt is null or undefined, moment will return the current date and time, which may not be the intended behavior.
https://topcoder.atlassian.net/browse/PM-1428