PM- 3026 Update Manage Submissions to accommodate AI workflows #7150
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| REVIEW_APP_URL: 'https://review.topcoder-dev.com', | ||
|
|
||
|
|
There was a problem hiding this comment.
[💡 style]
There are two consecutive blank lines here. While this is not a correctness issue, it could be considered a minor style inconsistency. Consider removing one of the blank lines to maintain a consistent style throughout the file.
| function loadAiWorkflowRunsInit() {} | ||
|
|
||
| function loadAiWorkflowRunsDone(tokenV3, submissionId, aiWorkflowId) { | ||
| const api = new Api(config.API.V6, tokenV3); |
There was a problem hiding this comment.
[❗❗ security]
Consider validating tokenV3, submissionId, and aiWorkflowId before using them to ensure they are not undefined or null. This can prevent potential runtime errors or security issues.
| const url = `/workflows/${aiWorkflowId}/runs?submissionId=${submissionId}`; | ||
|
|
||
| return api.get(url) | ||
| .then(res => res.json()) |
There was a problem hiding this comment.
[correctness]
The use of .then(res => res.json()) assumes that the response will always be JSON. Consider adding error handling for cases where the response might not be JSON, which could lead to runtime errors.
| aiWorkflowId, | ||
| runs: data, | ||
| })) | ||
| .catch((err) => { |
There was a problem hiding this comment.
[💡 maintainability]
Throwing the error directly without any additional context might make debugging difficult. Consider logging the error or adding more context to the error message.
| 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.
[security]
Consider using encodeURIComponent for challenge.id when constructing the URL to ensure that any special characters are properly encoded. This can prevent potential issues with malformed URLs.
| <Tooltip content={() => <div styleName="tooltip-content">Show Scores</div>}> | ||
| <Tooltip content={() => <div styleName="tooltip-content">View Review Info</div>}> | ||
| <button | ||
| onClick={() => onOpenReviewApp()} |
There was a problem hiding this comment.
[correctness]
The button for opening the review app is only rendered if safeForDownloadCheck is true. Ensure that this condition is intentional and that the review app should only be accessible when the submission is safe for download.
| 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 shape to ensure that the component receives the expected structure, which can prevent runtime errors and improve maintainability.
|
|
||
| SubmissionsTable.propTypes = { | ||
| challenge: PT.shape().isRequired, | ||
| submissionWorkflowRuns: PT.shape(), |
There was a problem hiding this comment.
[maintainability]
The submissionWorkflowRuns prop type is defined as PT.shape(), which is not specific enough. Consider defining the shape of this object to ensure that the expected structure is enforced, improving maintainability and reducing potential runtime errors.
| onlineReviewUrl: '', | ||
| helpPageUrl: '', | ||
| getSubmissionScores: _.noop, | ||
| submissionWorkflowRuns: [], |
There was a problem hiding this comment.
[❗❗ correctness]
The default value for submissionWorkflowRuns is set to an empty array, but its prop type is defined as a shape. This mismatch could lead to unexpected behavior. Consider setting the default value to an empty object {} to align with the prop type.
| padding-top: 0; | ||
|
|
||
| .workflow-table { | ||
| padding-left: 80px; |
There was a problem hiding this comment.
[maintainability]
The padding-left value of 80px for .workflow-table might cause layout issues on smaller screens. Consider using a relative unit like % or em to ensure better responsiveness across different screen sizes.
| }; | ||
|
|
||
| TableWorkflowRuns.propTypes = { | ||
| workflowRuns: PT.shape(), |
There was a problem hiding this comment.
[maintainability]
The workflowRuns prop type should be more specific. Instead of PT.shape(), consider defining the expected shape with PT.objectOf(PT.shape({ ... })) to improve type safety and clarity.
| 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 stylesheet harder to maintain and override in the future. Consider refactoring the CSS to achieve the desired effect without !important.
| } = this.props; | ||
| const { initialState } = this.state; | ||
|
|
||
| if (!challenge || !Array.isArray(challenge.reviewers) || !mySubmissions) return; |
There was a problem hiding this comment.
[correctness]
The early return here prevents the loadAiWorkflowRuns function from being called if mySubmissions is falsy. This could be intentional, but ensure that this behavior is desired, as it might skip loading AI workflow runs for valid submissions.
| if (!reviewer.aiWorkflowId) return; | ||
|
|
||
| const key = `${submission.id}-${reviewer.aiWorkflowId}`; | ||
| if (this.loadedWorkflowKeys.has(key)) return; |
There was a problem hiding this comment.
[💡 maintainability]
Consider logging or handling the case where a key is already present in loadedWorkflowKeys. This could help in debugging or understanding why certain AI workflow runs are not being loaded.
| dispatch(a.getSubmissionsDone(challengeId, tokens.tokenV3)); | ||
| }, | ||
|
|
||
| loadAiWorkflowRuns: (tokens, submissionId, aiWorkflowId) => { |
There was a problem hiding this comment.
[design]
The loadAiWorkflowRuns function dispatches two actions sequentially. Ensure that the order of these actions is correct and that any dependencies between them are handled properly.
| toBeDeletedId: payload, | ||
| deletionSucceed: true, | ||
| }), | ||
| [a.loadAiWorkflowRunsInit]: state => ({ ...state, isLoadingDetails: true }), |
There was a problem hiding this comment.
[💡 readability]
Consider renaming isLoadingDetails to something more descriptive like isLoadingAiWorkflowRuns to clearly indicate what is being loaded. This improves readability and maintainability by making the state purpose explicit.
| ...state, | ||
| submissionWorkflowRuns: { | ||
| ...state.submissionWorkflowRuns, | ||
| [submissionId]: runs, |
There was a problem hiding this comment.
[correctness]
Ensure that submissionId is always a valid key for submissionWorkflowRuns to avoid potential runtime errors. Consider adding validation or default handling for unexpected submissionId values.
| }; | ||
|
|
||
| export default function TableWorkflowRuns(props) { | ||
| const { workflowRuns } = props; |
There was a problem hiding this comment.
[❗❗ correctness]
The challengeId prop has been removed, but it is still referenced in the unchanged context of the code. Ensure that all references to challengeId are removed to prevent potential runtime errors.
| }; | ||
|
|
||
| TableWorkflowRuns.propTypes = { | ||
| workflowRuns: PT.shape(), |
There was a problem hiding this comment.
[maintainability]
The workflowRuns prop type is defined as PT.shape(), which is too generic and does not specify the expected structure of the workflowRuns object. Consider defining a more specific shape to improve type safety and maintainability.
| 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 runtime errors if TableWorkflowRuns does not handle null values gracefully. Consider providing a default value or ensuring TableWorkflowRuns can handle null inputs.
kkartunov
left a comment
kkartunov
left a comment
There was a problem hiding this comment.
Clean work. Looks good
| } | ||
| } | ||
|
|
||
| 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.
| type="button" | ||
| styleName="review-button" | ||
| > | ||
| Review |
There was a problem hiding this comment.
[correctness]
Consider adding a condition to disable the 'Review' button if safeForDownloadCheck is not true. This would maintain consistency with other buttons that are conditionally rendered based on safeForDownloadCheck.
| .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 unit (e.g., px, rem, em) instead of small for font-size to ensure consistent rendering across different browsers and platforms.
| return; | ||
| } | ||
|
|
||
| if (mySubmissions.length && !this.firstSubmissionExpanded) { |
There was a problem hiding this comment.
[correctness]
The logic to expand the first submission is directly modifying the component's state without using setState. This can lead to inconsistencies and unexpected behavior in React's state management. Consider using setState to update firstSubmissionExpanded to ensure React's state management is correctly utilized.
The AI workflow table will be hidden in mobile view as it was getting too cluttered.