Marathon match submission management features #1308
Conversation
Merge in AI changes to this branch
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| timestamp: new Date() | ||
| .toISOString(), | ||
| topic: 'avscan.action.scan', | ||
| topic: EnvironmentConfig.ADMIN.AVSCAN_TOPIC, |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that EnvironmentConfig.ADMIN.AVSCAN_TOPIC is always defined and correctly configured in all environments. If this configuration is missing or incorrect, it could lead to runtime errors or misrouted events.
| import SubmissionTableActionsNonMM from './SubmissionTableActionsNonMM' | ||
| import styles from './SubmissionTable.module.scss' | ||
|
|
||
| const renderVirusScanStatus = (submission: Submission): JSX.Element => { |
There was a problem hiding this comment.
[💡 correctness]
Consider handling the case where submission.virusScan is null or undefined explicitly. Currently, it defaults to displaying --, but it might be clearer to explicitly check for these cases.
| type: 'text', | ||
| }, | ||
| { | ||
| label: 'Virus Scan', |
There was a problem hiding this comment.
[performance]
Ensure that the renderVirusScanStatus function is performant enough for large datasets, as it will be called for each row in the table. Consider memoizing the result if the computation is non-trivial.
| </li> | ||
| <li | ||
| className={classNames({ | ||
| disabled: props.isDownloading[props.data.id], |
There was a problem hiding this comment.
[correctness]
The classNames function is used to conditionally apply the disabled class based on props.isDownloading[props.data.id]. Ensure that props.isDownloading is always defined and that props.data.id is a valid key to avoid potential runtime errors.
| </li> | ||
| <li | ||
| className={classNames({ | ||
| disabled: props.isDoingAvScan[props.data.id], |
There was a problem hiding this comment.
[correctness]
The classNames function is used to conditionally apply the disabled class based on props.isDoingAvScan[props.data.id]. Ensure that props.isDoingAvScan is always defined and that props.data.id is a valid key to avoid potential runtime errors.
| .inner { | ||
| max-width: $xl-max; | ||
| margin: 0 auto; | ||
| width: 100%; |
There was a problem hiding this comment.
[design]
Changing max-width: $xl-max; to width: 100%; may affect the layout by stretching the .inner element to the full width of its container. Ensure this change is intentional and that it doesn't negatively impact the design across different screen sizes.
| // Enriched fields from API (Admin/Copilot/M2M only) | ||
| submitterHandle?: string | ||
| submitterMaxRating?: number | null | ||
| virusScan?: boolean |
There was a problem hiding this comment.
[design]
The addition of the virusScan field to the Submission interface should be carefully considered. If this field is intended to represent the result of a virus scan, it might be more appropriate to use a more descriptive type, such as an enum or an object, to capture the scan status and any relevant details. Using a boolean may limit the expressiveness of this field and could lead to ambiguity about what true or false represents.
| bucket && bucket.toLowerCase() | ||
| .endsWith('-dmz'), | ||
| ) | ||
| const allowQuarantineRescan = submissionInfo.virusScan === false |
There was a problem hiding this comment.
[correctness]
The allowQuarantineRescan logic relies on submissionInfo.virusScan being exactly false. Consider using a more explicit check or handling potential undefined values to avoid unexpected behavior.
| bucket, | ||
| }: ValidateS3URIResult = validateS3URI(url) | ||
| const isQuarantineBucket = Boolean( | ||
| bucket && bucket.toLowerCase() |
There was a problem hiding this comment.
[correctness]
The use of toLowerCase() on bucket assumes it is always a string. Ensure bucket is validated or handled if it might be null or undefined to prevent runtime errors.
| isValid, | ||
| key: parsedKey, | ||
| } | ||
| } catch (error) {} |
There was a problem hiding this comment.
[maintainability]
The catch block is currently empty, which means any errors thrown by AmazonS3URI(fileURL) will be silently ignored. Consider logging the error or handling it appropriately to aid in debugging and ensure that unexpected issues don't go unnoticed.
| AWS_CLEAN_BUCKET: '', | ||
| AWS_DMZ_BUCKET: 'topcoder-dev-submissions', | ||
| AWS_QUARANTINE_BUCKET: '', | ||
| AWS_CLEAN_BUCKET: 'topcoder-dev-submissions', |
There was a problem hiding this comment.
[❗❗ security]
The AWS_CLEAN_BUCKET is now set to topcoder-dev-submissions. Ensure that this change is intentional and that the bucket permissions and policies are correctly configured to prevent unauthorized access or data leakage.
| AWS_DMZ_BUCKET: 'topcoder-dev-submissions', | ||
| AWS_QUARANTINE_BUCKET: '', | ||
| AWS_CLEAN_BUCKET: 'topcoder-dev-submissions', | ||
| AWS_DMZ_BUCKET: 'topcoder-dev-submissions-dmz', |
There was a problem hiding this comment.
[❗❗ security]
The AWS_DMZ_BUCKET has been changed to topcoder-dev-submissions-dmz. Verify that the bucket is correctly set up to handle DMZ requirements, such as restricted access and appropriate network configurations.
| AWS_QUARANTINE_BUCKET: '', | ||
| AWS_CLEAN_BUCKET: 'topcoder-dev-submissions', | ||
| AWS_DMZ_BUCKET: 'topcoder-dev-submissions-dmz', | ||
| AWS_QUARANTINE_BUCKET: 'topcoder-dev-submissions-quarantine', |
There was a problem hiding this comment.
[❗❗ security]
The AWS_QUARANTINE_BUCKET is now set to topcoder-dev-submissions-quarantine. Ensure that this bucket is configured with the necessary security measures to isolate potentially harmful files and prevent them from affecting other systems.
| AWS_CLEAN_BUCKET: '', | ||
| AWS_DMZ_BUCKET: 'topcoder-submissions', | ||
| AWS_QUARANTINE_BUCKET: '', | ||
| AWS_CLEAN_BUCKET: 'topcoder-submissions', |
There was a problem hiding this comment.
[❗❗ correctness]
The AWS_CLEAN_BUCKET and AWS_DMZ_BUCKET are both set to topcoder-submissions. Ensure this is intentional, as using the same bucket for different purposes could lead to data management issues.
| className={classNames( | ||
| styles.content, | ||
| props.contentClass, | ||
| { [styles.isFluid]: props.isFluid }, |
There was a problem hiding this comment.
[correctness]
The use of styles.isFluid within classNames is correct, but ensure that styles.isFluid is defined in ContentLayout.module.scss to avoid potential runtime issues.
Better support for MM submissions - rescanning / rescoring