Skip to content
Merged

Pm 2179 #1315

Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
@import '@libs/ui/styles/includes';

.tableCell {
vertical-align: middle;
}

.filenameCell {
display: flex;
align-items: center;
gap: 6px;
color: $link-blue-dark;
cursor: pointer;
opacity: 1;
transition: opacity 0.15s ease;

&:hover {
text-decoration: underline;
}
}

.downloading {
cursor: wait;
opacity: 0.6;
}

.expired {
cursor: not-allowed;
color: #999;

&:hover {
text-decoration: none;
}
}

.artifactType {
display: flex;
gap: 5px;
align-items: center;

.artifactIcon {
stroke: #00797A;
}
}
Original file line number Diff line number Diff line change
@@ -1,15 +1,137 @@
import { FC } from 'react'
import { FC, useCallback, useMemo } from 'react'
import { noop } from 'lodash'
import classNames from 'classnames'
import moment from 'moment'

// import styles from './ScorecardAttachments.module.scss'
import { IconOutline, Table, TableColumn } from '~/libs/ui'
import { useReviewsContext } from '~/apps/review/src/pages/reviews/ReviewsContext'

import { AiWorkflowRunArtifact,
AiWorkflowRunArtifactDownloadResponse,
AiWorkflowRunAttachmentsResponse,
useDownloadAiWorkflowsRunArtifact, useFetchAiWorkflowsRunAttachments } from '../../../hooks'
import { TableWrapper } from '../../TableWrapper'
import { TABLE_DATE_FORMAT } from '../../../constants'
import { formatFileSize } from '../../common'
import { ReviewsContextModel } from '../../../models'

import styles from './ScorecardAttachments.module.scss'

interface ScorecardAttachmentsProps {
className?: string
}

const ScorecardAttachments: FC<ScorecardAttachmentsProps> = props => (
<div className={props.className}>
attachments
</div>
)
const ScorecardAttachments: FC<ScorecardAttachmentsProps> = (props: ScorecardAttachmentsProps) => {
const className = props.className
// const { width: screenWidth }: WindowSize = useWindowSize()
// const isTablet = useMemo(() => screenWidth <= 1000, [screenWidth])
const { workflowId, workflowRun }: ReviewsContextModel = useReviewsContext()
const { artifacts }: AiWorkflowRunAttachmentsResponse
= useFetchAiWorkflowsRunAttachments(workflowId, workflowRun?.id)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
Consider adding error handling for the useFetchAiWorkflowsRunAttachments hook to manage cases where the fetch might fail. This will improve the robustness of the component.

const { download, isDownloading }: AiWorkflowRunArtifactDownloadResponse = useDownloadAiWorkflowsRunArtifact(
workflowId,
workflowRun?.id,
)

const handleDownload = useCallback(
async (artifactId: number): Promise<void> => {
await download(artifactId)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
The handleDownload function should include error handling to manage potential failures during the download process. This will ensure that the user is informed if something goes wrong.

},
[download],
)

const createDownloadHandler = useCallback(
(id: number) => () => handleDownload(id),
[handleDownload],
)

const columns = useMemo<TableColumn<AiWorkflowRunArtifact>[]>(
() => [
{
className: classNames(styles.tableCell),
label: 'Filename',
propertyName: 'name',
renderer: (attachment: AiWorkflowRunArtifact) => {
const isExpired = attachment.expired

return (
<div
className={classNames(
styles.filenameCell,
{
[styles.expired]: isExpired,
[styles.downloading]: isDownloading && !isExpired,
},
)}
onClick={!isExpired ? createDownloadHandler(attachment.id) : undefined}
>
<span>{attachment.name}</span>
{isExpired && <span>(Link Expired)</span>}
</div>
)
},
type: 'element',
},
{
className: classNames(styles.tableCell),
label: 'Type',
renderer: () => (
<div className={styles.artifactType}>
<IconOutline.CubeIcon className={styles.artifactIcon} width={26} />
<span>Artifact</span>
</div>
),
type: 'element',
},
{
className: classNames(styles.tableCell),
label: 'Size',
propertyName: 'sizeInBytes',
renderer: (attachment: AiWorkflowRunArtifact) => (
<div>{formatFileSize(attachment.size_in_bytes)}</div>
),
type: 'element',
},
{
className: styles.tableCell,
label: 'Attached Date',
renderer: (attachment: AiWorkflowRunArtifact) => (
<span className='last-element'>
{moment(attachment.created_at)
.local()
.format(TABLE_DATE_FORMAT)}
</span>
),
type: 'element',
},
],
[],
)

return (
<TableWrapper
className={classNames(
styles.container,
className,
'enhanced-table',
)}
>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 design]
Consider adding a loading state or indicator while the attachments are being fetched. This will enhance the user experience by providing feedback that the data is being loaded.

{artifacts ? (
<Table
columns={columns}
data={artifacts}
disableSorting
onToggleSort={noop}
removeDefaultSort
className='enhanced-table-desktop'
/>
) : (
<div>No attachments</div>
)}

</TableWrapper>
)

}

export default ScorecardAttachments
25 changes: 25 additions & 0 deletions src/apps/review/src/lib/components/common/columnUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,28 @@ export const getProfileUrl = (handle: string): string => {

return `${normalizedBase}/${encodeURIComponent(handle)}`
}

/**
* converts size_in_bytes into KB / MB / GB with correct formatting.
*/
export const formatFileSize = (bytes: number): string => {
if (!bytes || bytes < 0) return '0 B'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ correctness]
The check !bytes will return true for 0, which is not a negative value. Consider changing the condition to bytes <= 0 to handle only negative values and zero correctly.


const KB = 1024
const MB = KB * 1024
const GB = MB * 1024

if (bytes >= GB) {
return `${(bytes / GB).toFixed(2)} GB`
}

if (bytes >= MB) {
return `${(bytes / MB).toFixed(2)} MB`
}

if (bytes >= KB) {
return `${(bytes / KB).toFixed(2)} KB`
}

return `${bytes} B`
}
1 change: 1 addition & 0 deletions src/apps/review/src/lib/constants.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export const SUBMISSION_TYPE_CONTEST = 'CONTEST_SUBMISSION'
export const SUBMISSION_TYPE_CHECKPOINT = 'CHECKPOINT_SUBMISSION'
export const TABLE_DATE_FORMAT = 'MMM DD, HH:mm A'
111 changes: 109 additions & 2 deletions src/apps/review/src/lib/hooks/useFetchAiWorkflowRuns.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { useEffect } from 'react'
import { useEffect, useState } from 'react'
import useSWR, { SWRResponse } from 'swr'

import { EnvironmentConfig } from '~/config'
import { xhrGetAsync } from '~/libs/core'
import { xhrGetAsync, xhrGetBlobAsync } from '~/libs/core'
import { handleError } from '~/libs/shared/lib/utils/handle-error'

import { AiFeedbackItem, Scorecard } from '../models'
Expand Down Expand Up @@ -46,6 +46,23 @@ export interface AiWorkflowRun {
workflow: AiWorkflow
}

export interface AiWorkflowRunArtifact {
id: number
name: string
size_in_bytes: number
url: string
archive_download_url: string
expired: boolean
workflow_run: {
id: number
repository_id: number
head_sha: string
}
created_at: string
updated_at: string
expires_at: string
}

export type AiWorkflowRunItem = AiFeedbackItem

const TC_API_BASE_URL = EnvironmentConfig.API.V6
Expand All @@ -60,6 +77,22 @@ export interface AiWorkflowRunItemsResponse {
isLoading: boolean
}

export interface AiWorkflowRunAttachmentsApiResponse {
artifacts: AiWorkflowRunArtifact[]
total_count: number
}

export interface AiWorkflowRunAttachmentsResponse {
artifacts: AiWorkflowRunArtifact[]
totalCount: number
isLoading: boolean
}

export interface AiWorkflowRunArtifactDownloadResponse {
download: (artifactId: number) => Promise<void>
isDownloading: boolean
}

export const aiRunInProgress = (aiRun: Pick<AiWorkflowRun, 'status'>): boolean => [
AiWorkflowRunStatusEnum.INIT,
AiWorkflowRunStatusEnum.QUEUED,
Expand Down Expand Up @@ -137,3 +170,77 @@ export function useFetchAiWorkflowsRunItems(
runItems,
}
}

export function useFetchAiWorkflowsRunAttachments(
workflowId?: string,
runId?: string | undefined,
): AiWorkflowRunAttachmentsResponse {
const {
data,
error: fetchError,
isValidating: isLoading,
}: SWRResponse<AiWorkflowRunAttachmentsApiResponse, Error> = useSWR<
AiWorkflowRunAttachmentsApiResponse,
Error
>(
`${TC_API_BASE_URL}/workflows/${workflowId}/runs/${runId}/attachments`,
{
isPaused: () => !workflowId || !runId,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
The isPaused option in the useSWR hook is used to conditionally pause the request. However, the current logic will not pause if workflowId or runId is an empty string. Consider using !workflowId?.trim() || !runId?.trim() to ensure that empty strings also pause the request.

},
)

useEffect(() => {
if (fetchError) {
handleError(fetchError)
}
}, [fetchError])

return {
artifacts: data?.artifacts ?? [],
isLoading,
totalCount: data?.total_count ?? 0,
}
}

export function useDownloadAiWorkflowsRunArtifact(
workflowId?: string,
runId?: string | undefined,
): AiWorkflowRunArtifactDownloadResponse {
const [isDownloading, setIsDownloading] = useState(false)

const download = async (artifactId: number): Promise<void> => {
if (!workflowId || !runId || !artifactId) return

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ correctness]
The check if (!workflowId || !runId || !artifactId) return does not handle the case where artifactId is zero, which is a valid number. Consider using artifactId == null to check for null or undefined values.


try {
setIsDownloading(true)

const url = `${TC_API_BASE_URL}/workflows/${workflowId}/runs/${runId}/attachments/${artifactId}/zip`

xhrGetBlobAsync<Blob>(url)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@himaniraghav3 please conver to async/await type of code block. In general, we have to avoid mixing .then/.catch with async/await. The function is async so we should await. This will also ensure we do not end up with unhandled promises as noted for line 238.

.then(blob => {
const objectUrl = window.URL.createObjectURL(blob)
const link = document.createElement('a')
link.href = objectUrl
link.download = `artifact-${artifactId}.zip`

document.body.appendChild(link)
link.click()
link.remove()

window.URL.revokeObjectURL(objectUrl)
})
.catch(err => {
handleError(err as Error)
})
} catch (err) {
handleError(err as Error)
} finally {
setIsDownloading(false)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ correctness]
The setIsDownloading(false) call in the finally block will execute after the asynchronous xhrGetBlobAsync call, potentially setting isDownloading to false before the download completes. Consider moving setIsDownloading(false) inside the .then and .catch blocks to ensure it reflects the actual download state.

}
}

return {
download,
isDownloading,
}
}
Loading