Pm 2179 #1315
Pm 2179 #1315
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
...ps/review/src/lib/components/Scorecard/ScorecardAttachments/ScorecardAttachments.module.scss
Outdated
Show resolved
Hide resolved
| // const isTablet = useMemo(() => screenWidth <= 1000, [screenWidth]) | ||
| const { workflowId, workflowRun }: ReviewsContextModel = useReviewsContext() | ||
| const { artifacts }: AiWorkflowRunAttachmentsResponse | ||
| = useFetchAiWorkflowsRunAttachments(workflowId, workflowRun?.id) |
There was a problem hiding this comment.
[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 handleDownload = useCallback( | ||
| async (artifactId: number): Promise<void> => { | ||
| await download(artifactId) |
There was a problem hiding this comment.
[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.
| className, | ||
| 'enhanced-table', | ||
| )} | ||
| > |
There was a problem hiding this comment.
[💡 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.
| * converts size_in_bytes into KB / MB / GB with correct formatting. | ||
| */ | ||
| export const formatFileSize = (bytes: number): string => { | ||
| if (!bytes || bytes < 0) return '0 B' |
There was a problem hiding this comment.
[❗❗ 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.
| >( | ||
| `${TC_API_BASE_URL}/workflows/${workflowId}/runs/${runId}/attachments`, | ||
| { | ||
| isPaused: () => !workflowId || !runId, |
There was a problem hiding this comment.
[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.
| const [isDownloading, setIsDownloading] = useState(false) | ||
|
|
||
| const download = async (artifactId: number): Promise<void> => { | ||
| if (!workflowId || !runId || !artifactId) return |
There was a problem hiding this comment.
[❗❗ 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.
| } catch (err) { | ||
| handleError(err as Error) | ||
| } finally { | ||
| setIsDownloading(false) |
There was a problem hiding this comment.
[❗❗ 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.
| @import '@libs/ui/styles/includes'; | ||
|
|
||
| .container { | ||
| min-height: calc($content-height - 250px); |
There was a problem hiding this comment.
[maintainability]
The use of a hardcoded value (250px) in the min-height calculation could lead to maintainability issues. Consider defining this value as a variable or constant to improve flexibility and ease of updates.
kkartunov
left a comment
kkartunov
left a comment
There was a problem hiding this comment.
@himaniraghav3 very good work!
Minor observation shared below.
Please also add attachments count to the tab heading and make sure it behaves on mobile.
|
|
||
| const url = `${TC_API_BASE_URL}/workflows/${workflowId}/runs/${runId}/attachments/${artifactId}/zip` | ||
|
|
||
| xhrGetBlobAsync<Blob>(url) |
There was a problem hiding this comment.
@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.
...ps/review/src/lib/components/Scorecard/ScorecardAttachments/ScorecardAttachments.module.scss
Show resolved
Hide resolved
src/apps/review/src/lib/components/Scorecard/ScorecardAttachments/ScorecardAttachments.tsx
Show resolved
Hide resolved
| [styles.expired]: isExpired, | ||
| [styles.downloading]: downloading, | ||
| })} | ||
| onClick={!isExpired ? createDownloadHandler(attachment.id) : undefined} |
There was a problem hiding this comment.
[💡 design]
The onClick handler for downloading an attachment is conditionally set to undefined if the attachment is expired. This is correct, but consider adding a cursor: not-allowed style to visually indicate that the download action is disabled for expired attachments, improving user experience.
src/apps/review/src/pages/reviews/components/AiReviewViewer/AiReviewViewer.tsx
Show resolved
Hide resolved
Added mobile view |
kkartunov
left a comment
kkartunov
left a comment
There was a problem hiding this comment.
Looks good. Thanks @himaniraghav3
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-2179
What's in this PR?