[WEB-2717] chore: implemented issue attachment upload progress #5901
Conversation
WalkthroughThe pull request introduces significant modifications to the attachment handling components in the web application. Key changes include the renaming and type adjustments of props related to attachment operations, the introduction of new components for upload status representation, and enhancements to existing components to improve the handling of file uploads. The overall structure is streamlined by utilizing a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (15)
web/core/components/issues/issue-detail-widgets/attachments/content.tsx (1)
19-25: Consider adding JSDoc comments for better documentationThe rename from
handleAttachmentOperationstoattachmentHelpersimproves clarity. While the changes are good, consider adding JSDoc comments to document the shape and purpose of theattachmentHelpersobject.+ /** Helper functions and state for managing issue attachments and upload progress */ const attachmentHelpers = useAttachmentOperations(workspaceSlug, projectId, issueId);web/core/services/file-upload.service.ts (1)
24-24: Consider adding type safety for upload progress eventsThe progress handler implementation could benefit from stronger typing.
// Add this type at the top of the file type UploadProgressEvent = { loaded: number; total?: number; progress?: number; };This would help consumers of the service better understand the shape of progress events they'll receive.
web/core/components/issues/attachment/root.tsx (1)
Line range hint
1-37: Well-structured architecture for upload progress trackingThe architectural changes demonstrate good practices:
- Using MobX for reactive state management
- Consolidating attachment logic in a custom hook
- Clear separation of concerns between upload and list components
This structure will make it easier to maintain and extend the upload progress functionality.
web/core/components/issues/attachment/attachments-list.tsx (1)
30-32: Consider handling empty states.The upload status implementation looks good, but consider handling empty states for better UX.
Consider adding empty state handling:
return ( <> + {(!uploadStatus?.length && !issueAttachments?.length) && ( + <div className="text-custom-text-400 text-sm"> + No attachments yet + </div> + )} {uploadStatus?.map((uploadStatus) => ( <IssueAttachmentsUploadDetails key={uploadStatus.id} uploadStatus={uploadStatus} /> ))}Also applies to: 38-38
web/core/services/api.service.ts (1)
31-52: LGTM! Enhanced type safety for HTTP methods.The addition of
AxiosRequestConfigtyping improves type safety and enables proper typing for upload progress callbacks, which is essential for the attachment upload feature.Consider improving type safety further by replacing
anywith a more specific type for thedataparameter in the delete method:- delete(url: string, data?: any, config: AxiosRequestConfig = {}) { + delete<T = any>(url: string, data?: T, config: AxiosRequestConfig = {}) {web/core/components/issues/attachment/attachment-list-upload-item.tsx (1)
19-45: Extract magic numbers as constants.Consider extracting hard-coded values to named constants for better maintainability.
+const ICON_SIZE = 18; +const PROGRESS_INDICATOR_SIZE = 20; +const PROGRESS_INDICATOR_STROKE_WIDTH = 3; export const IssueAttachmentsUploadItem: React.FC<Props> = observer((props) => { // ... - const fileIcon = getFileIcon(fileExtension, 18); + const fileIcon = getFileIcon(fileExtension, ICON_SIZE); // ... - <CircularProgressIndicator size={20} strokeWidth={3} percentage={uploadStatus.progress} /> + <CircularProgressIndicator + size={PROGRESS_INDICATOR_SIZE} + strokeWidth={PROGRESS_INDICATOR_STROKE_WIDTH} + percentage={uploadStatus.progress} + />web/core/components/issues/attachment/attachment-upload.tsx (2)
Line range hint
18-32: Implementation missing upload progress tracking.While the component handles the upload operation and loading state, it doesn't implement the progress tracking mentioned in the PR objectives. Consider enhancing the implementation to show upload progress.
Here's a suggested implementation using XMLHttpRequest or Axios for progress tracking:
const onDrop = useCallback( (acceptedFiles: File[]) => { const currentFile: File = acceptedFiles[0]; if (!currentFile || !workspaceSlug) return; setIsLoading(true); - attachmentOperations.create(currentFile).finally(() => setIsLoading(false)); + const onProgress = (progress: number) => { + // Update progress state + setUploadProgress(progress); + }; + + attachmentOperations.create(currentFile, onProgress).finally(() => { + setIsLoading(false); + setUploadProgress(0); + }); }, - [attachmentOperations, workspaceSlug] + [attachmentOperations, workspaceSlug, setUploadProgress] );
Line range hint
44-64: Enhance UI to show upload progress.The UI currently only shows a "Uploading..." message. To meet the PR objectives, consider adding a progress indicator.
Here's a suggested UI enhancement:
<span className="flex items-center gap-2"> {isDragActive ? ( <p>Drop here...</p> ) : fileError ? ( <p className="text-center text-red-500">{fileError}</p> ) : isLoading ? ( - <p className="text-center">Uploading...</p> + <div className="flex flex-col items-center gap-1 w-full"> + <p className="text-center">Uploading... {uploadProgress}%</p> + <div className="w-full h-1 bg-custom-border-200 rounded-full overflow-hidden"> + <div + className="h-full bg-custom-primary transition-all duration-300" + style={{ width: `${uploadProgress}%` }} + /> + </div> + </div> ) : ( <p className="text-center">Click or drag a file here</p> )} </span>web/core/components/issues/issue-detail-widgets/attachments/quick-action-button.tsx (2)
Line range hint
44-54: Consider adding upload progress feedback.While the file upload functionality works correctly, the PR's objective mentions implementing upload progress indication. Consider adding progress feedback using the attachment operations to improve user experience.
Here's a suggested implementation:
attachmentOperations - .create(currentFile) + .create(currentFile, { + onProgress: (progress) => { + // Update UI with upload progress + setUploadProgress(progress); + } + }) .catch(() => {This would require adding:
- A progress state:
const [uploadProgress, setUploadProgress] = useState(0);- UI elements to display the progress during upload
Line range hint
82-87: Address the TODO comment regarding event propagation.The current event propagation handling through an extra div is not ideal. Consider refactoring this to handle the event directly in the button component.
Here's a suggested implementation:
- <div - onClick={(e) => { - // TODO: Remove extra div and move event propagation to button - e.stopPropagation(); - }} - > - <button {...getRootProps()} type="button" disabled={disabled}> + <button + {...getRootProps()} + type="button" + disabled={disabled} + onClick={(e) => { + e.stopPropagation(); + getRootProps().onClick?.(e); + }} + > <input {...getInputProps()} /> {customButton ? customButton : <Plus className="h-4 w-4" />} - </button> - </div> + </button>web/core/components/issues/attachment/attachment-detail.tsx (1)
Line range hint
1-95: Consider splitting the component for better maintainability.While the changes look good, consider the following improvements:
- Extract the file metadata display logic into a separate component
- Move magic numbers (e.g., truncateText length of 10) to constants
- Consider using a custom hook for file metadata calculations
This would improve maintainability and make the component more focused.
Example refactor for the file metadata component:
type FileMetadataProps = { fileName: string; fileExtension: string; fileIcon: React.ReactNode; uploadInfo: { uploaderName: string; uploadDate: string; }; size: number; }; const FILE_NAME_MAX_LENGTH = 10; const FileMetadata: FC<FileMetadataProps> = ({ fileName, fileExtension, fileIcon, uploadInfo, size, }) => ( <div className="flex flex-col gap-1"> <div className="flex items-center gap-2"> <Tooltip tooltipContent={fileName} isMobile={isMobile}> <span className="text-sm"> {truncateText(fileName, FILE_NAME_MAX_LENGTH)} </span> </Tooltip> <Tooltip isMobile={isMobile} tooltipContent={`${uploadInfo.uploaderName} uploaded on ${uploadInfo.uploadDate}`} > <span> <AlertCircle className="h-3 w-3" /> </span> </Tooltip> </div> <div className="flex items-center gap-3 text-xs text-custom-text-200"> <span>{fileExtension.toUpperCase()}</span> <span>{convertBytesToSize(size)}</span> </div> </div> );web/core/components/issues/attachment/attachment-item-list.tsx (1)
86-113: LGTM: Well-implemented upload progress UI with room for minor enhancement.The UI implementation effectively shows upload progress and maintains good UX. However, consider improving the disabled state visibility.
Consider enhancing the disabled state styling:
-className={`relative flex flex-col ${isDragActive && issueAttachments.length < 3 ? "min-h-[200px]" : ""} ${disabled ? "cursor-not-allowed" : "cursor-pointer"}`} +className={`relative flex flex-col ${isDragActive && issueAttachments.length < 3 ? "min-h-[200px]" : ""} ${ + disabled ? "cursor-not-allowed opacity-60" : "cursor-pointer" +}`}web/core/components/issues/issue-detail-widgets/attachments/helper.tsx (2)
Line range hint
36-69: Refactor to eliminate duplicate validation logicBoth the
createandremovemethods perform the same validation check forworkspaceSlug,projectId, andissueId. To enhance maintainability and reduce code duplication, consider extracting this validation into a separate helper function.Here's how you might refactor the code:
const validateRequiredFields = () => { if (!workspaceSlug || !projectId || !issueId) { throw new Error("Missing required fields"); } }; const attachmentOperations: TAttachmentOperations = useMemo( () => ({ create: async (file) => { - if (!workspaceSlug || !projectId || !issueId) throw new Error("Missing required fields"); + validateRequiredFields(); // Rest of the create logic... }, remove: async (attachmentId) => { - if (!workspaceSlug || !projectId || !issueId) throw new Error("Missing required fields"); + validateRequiredFields(); // Rest of the remove logic... }, }), [/* dependencies */] );
66-66: Ensure consistent error handling increateandremovemethodsIn the
createmethod, the caught error is rethrown after logging, allowing upstream handlers to deal with it. In contrast, theremovemethod does not rethrow the error after catching it. For consistency and better error propagation, consider rethrowing the error in theremovemethod as well.Apply this diff to rethrow the error in the
removemethod:} catch (error) { captureIssueEvent({ eventName: "Issue attachment deleted", payload: { id: issueId, state: "FAILED", element: "Issue detail page" }, updates: { changed_property: "attachment", change_details: "", }, }); setToast({ message: "The Attachment could not be removed", type: TOAST_TYPE.ERROR, title: "Attachment not removed", }); + throw error; }Also applies to: 90-102
web/core/store/issue/issue-details/attachment.store.ts (1)
167-168: Enhance error handling in the upload processCurrently, errors during the upload are logged to the console and re-thrown. Consider providing user feedback to inform them of the upload failure, improving the user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
- web/core/components/issues/attachment/attachment-detail.tsx (2 hunks)
- web/core/components/issues/attachment/attachment-item-list.tsx (3 hunks)
- web/core/components/issues/attachment/attachment-list-upload-item.tsx (1 hunks)
- web/core/components/issues/attachment/attachment-upload-details.tsx (1 hunks)
- web/core/components/issues/attachment/attachment-upload.tsx (2 hunks)
- web/core/components/issues/attachment/attachments-list.tsx (1 hunks)
- web/core/components/issues/attachment/delete-attachment-modal.tsx (2 hunks)
- web/core/components/issues/attachment/index.ts (1 hunks)
- web/core/components/issues/attachment/root.tsx (3 hunks)
- web/core/components/issues/issue-detail-widgets/attachments/content.tsx (2 hunks)
- web/core/components/issues/issue-detail-widgets/attachments/helper.tsx (3 hunks)
- web/core/components/issues/issue-detail-widgets/attachments/quick-action-button.tsx (3 hunks)
- web/core/components/issues/issue-detail-widgets/issue-detail-widget-collapsibles.tsx (2 hunks)
- web/core/services/api.service.ts (2 hunks)
- web/core/services/file-upload.service.ts (2 hunks)
- web/core/services/issue/issue_attachment.service.ts (3 hunks)
- web/core/store/issue/issue-details/attachment.store.ts (6 hunks)
🧰 Additional context used
🪛 Biome
web/core/store/issue/issue-details/attachment.store.ts
[error] 155-155: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (38)
web/core/components/issues/attachment/index.ts (1)
4-5: LGTM! Well-organized exports for the new upload progress components.The new exports are properly organized, following the existing barrel file pattern and maintaining alphabetical order. They align well with the PR's objective of implementing attachment upload progress indicators.
web/core/components/issues/issue-detail-widgets/attachments/content.tsx (3)
3-3: LGTM: MobX integration for reactive updatesThe addition of the
observerimport is appropriate for enabling reactive updates to the upload progress state.
16-16: LGTM: Component correctly wrapped with observerThe component is properly wrapped with the
observerHOC, enabling it to react to changes in observable state, which is essential for real-time upload progress updates.
21-26: Verify prop type consistency across componentsLet's verify that the
attachmentHelpersprop type is consistent across related components.✅ Verification successful
Let me gather more information about the type definitions to ensure proper typing across components.
This will help us verify the type consistency and proper usage of attachmentHelpers across components.
Let me gather one final piece of information to verify the Props type definitions in the components where attachmentHelpers is used.
This will help us confirm the type consistency across all components using attachmentHelpers.
Let me try one more focused search to find the Props type definition for IssueAttachmentItemList.
This will help us verify the type consistency specifically for the IssueAttachmentItemList component.
Based on the gathered information, I can now provide the final response:
Props type consistency verified for attachmentHelpers
The
attachmentHelpersprop is properly typed and consistently used across components:
- The
IssueAttachmentItemListcomponent correctly defines it asTAttachmentHelperstype- The type
TAttachmentHelpersis properly defined inweb/core/components/issues/issue-detail-widgets/attachments/helper.tsx- The same type is consistently imported and used in related components:
- attachment-detail.tsx
- attachments-list.tsx
- attachment-item-list.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for IssueAttachmentItemList component definition and usages ast-grep --pattern 'type Props = { $$$ attachmentHelpers: $_ $$$ }' # Search for other components using attachmentHelpers rg -A 2 'attachmentHelpers.*='Length of output: 2401
Script:
#!/bin/bash # Search for attachmentHelpers type definition and the hook that creates it ast-grep --pattern 'useAttachmentOperations' # Search for Props type definitions in attachment-related components ast-grep --pattern 'interface Props { $$$ attachmentHelpers: $_ $$$ }' # Also search with 'type' keyword ast-grep --pattern 'type Props = { $$$ attachmentHelpers: $_ $$$ }' # Look for the actual type definition of attachmentHelpers rg "type.*AttachmentHelpers.*=" -A 3Length of output: 1640
Script:
#!/bin/bash # Search for Props definitions in the specific components we found earlier rg -B 2 -A 5 "Props.*{" web/core/components/issues/attachment/attachment-item-list.tsx web/core/components/issues/attachment/attachments-list.tsx web/core/components/issues/attachment/attachment-detail.tsx # Also check the imports of TAttachmentHelpers type rg "import.*TAttachmentHelpers"Length of output: 38638
Script:
#!/bin/bash # Search for IssueAttachmentItemList component and its Props definition rg -B 5 -A 10 "IssueAttachmentItemList" web/core/components/issues/attachment/attachment-item-list.tsx # Also search for TAttachmentHelpers usage rg "TAttachmentHelpers" --type tsLength of output: 1965
web/core/services/file-upload.service.ts (3)
1-1: LGTM: Clean import additionThe addition of
AxiosRequestConfigtype import is appropriate for typing the new progress handler parameter.
12-16: Verify error handling for the progress handlerThe method signature change looks good, but we should ensure proper error handling for the progress handler callback.
Consider wrapping the progress handler invocation in a try-catch block to prevent upload failures if the handler throws an error. Here's a suggested implementation:
async uploadFile( url: string, data: FormData, uploadProgressHandler?: AxiosRequestConfig["onUploadProgress"] ): Promise<void> { + const safeProgressHandler = uploadProgressHandler + ? (event: any) => { + try { + uploadProgressHandler(event); + } catch (error) { + console.error('Error in upload progress handler:', error); + } + } + : undefined; this.cancelSource = axios.CancelToken.source(); return this.post(url, data, { // ... other config - onUploadProgress: uploadProgressHandler, + onUploadProgress: safeProgressHandler, })
Line range hint
1-35: Verify integration with error handling systemThe implementation looks solid, but let's verify the error handling integration across the codebase.
✅ Verification successful
Based on the search results, I can now verify the error handling integration in the codebase. Let me provide the final response:
Error handling implementation is properly integrated across the codebase
The error handling in
file-upload.service.tsfollows the established patterns across the codebase:
- Proper error propagation with axios error handling:
.catch((error) => { if (axios.isCancel(error)) { console.log(error.message); } else { throw error?.response?.data; } })
- Consistent error handling with:
- Upload cancellation support via
cancelUpload()- Progress tracking via
onUploadProgresshandler- Error response data extraction
- Special handling for cancelled uploads
The implementation aligns with the error handling patterns used across other services in the codebase, making it consistent and reliable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling patterns in related files echo "Checking error handling patterns..." rg -A 5 "catch.*error" --type ts echo "Checking upload progress handler usage..." rg -A 5 "onUploadProgress" --type ts echo "Checking cancelUpload usage..." rg -A 5 "cancelUpload" --type tsLength of output: 305432
web/core/components/issues/attachment/root.tsx (4)
3-6: LGTM: Clean import restructuringThe imports have been streamlined to support the new reactive architecture using MobX and the consolidated attachment operations hook.
18-18: LGTM: Enhanced reactivity with MobX observerWrapping the component with
observerensures reactive updates to attachment upload status changes, which is crucial for implementing the upload progress indicator.
31-31: Verify prop usage in child componentsThe prop structure has been updated to support upload progress tracking. Let's verify the implementation in child components.
#!/bin/bash # Description: Check how child components use the new props # Expected: Components should properly handle upload progress # Check IssueAttachmentUpload implementation ast-grep --pattern 'const IssueAttachmentUpload = $_({ attachmentOperations }: $_) => { $$$ }' # Check IssueAttachmentsList implementation ast-grep --pattern 'const IssueAttachmentsList = $_({ attachmentHelpers }: $_) => { $$$ }'Also applies to: 33-33
22-22: Verify hook implementation detailsThe hook usage looks correct, but let's verify its implementation to ensure it properly handles upload progress tracking.
✅ Verification successful
Let me try different search patterns to locate the hook implementation and progress tracking logic.
Let's check the implementation in the helper file we found and look for progress tracking logic.
Let me check the implementation of
createAttachmentfunction since it's being used in the hook.
Hook implementation correctly handles upload progress tracking
The
useAttachmentOperationshook is properly implemented with upload progress tracking functionality. Here's what I found:
- The hook uses
createAttachmentfrom the store which includes progress tracking through:
- Maintains upload status in
attachmentsUploadStatusMapwith progress percentage- Provides progress updates through a callback in the upload service
- Exposes upload status through
getAttachmentsUploadStatusByIssueIdmethod- The hook returns both operations and snapshot of upload status:
return { operations: attachmentOperations, snapshot: { uploadStatus: attachmentsUploadStatus } };🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of useAttachmentOperations hook # Expected: Hook should handle upload progress tracking # Look for the hook implementation ast-grep --pattern 'export const useAttachmentOperations = $_' # Check for progress tracking logic rg -A 10 'uploadProgress|onProgress|progress' --type ts --type tsxLength of output: 193
Script:
#!/bin/bash # Search for the hook implementation with a broader pattern ast-grep --pattern 'useAttachmentOperations' # Search for the hook implementation file fd 'attachment.*operations' --type f # Look for upload-related logic with broader context rg -A 15 'upload.*attachment|attachment.*upload' --type ts --type tsxLength of output: 1193
Script:
#!/bin/bash # Check the helper file implementation cat "web/core/components/issues/issue-detail-widgets/attachments/helper.tsx" # Look for upload-related logic with correct file type rg -A 15 'upload.*attachment|attachment.*upload' --type typescript --type tsxLength of output: 4025
Script:
#!/bin/bash # Look for createAttachment implementation ast-grep --pattern 'createAttachment = async' # Check the store file for attachment implementation fd 'attachment.store' --type f --exec cat {}Length of output: 6924
web/core/components/issues/attachment/attachments-list.tsx (4)
5-6: LGTM! Clean import organization.The imports are well-structured and properly categorized with comments.
Also applies to: 9-9
18-18: LGTM! Clean state management structure.Good use of destructuring and clear separation between store and props usage.
Also applies to: 24-25
Line range hint
1-42: Verify integration with new components.Let's ensure proper integration with the upload details component and helpers.
✅ Verification successful
Let me gather more information about the implementation details of the related components.
Based on the gathered information, I can now provide the final response as the implementation details are clear.
Integration with upload components and helpers is properly implemented
The verification shows that:
IssueAttachmentsUploadDetailscorrectly handles upload status display through theuploadStatuspropTAttachmentHelperstype is consistently used across components and properly structured withoperationsandsnapshotproperties- Both list components (
IssueAttachmentsListandIssueAttachmentItemList) correctly implement the attachment helpers and upload status handling🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of related components # Expect: IssueAttachmentsUploadDetails should handle upload status display # Expect: TAttachmentHelpers should be used consistently # Check IssueAttachmentsUploadDetails implementation ast-grep --pattern 'const IssueAttachmentsUploadDetails = $$$' # Check consistent usage of attachmentHelpers rg -l "attachmentHelpers.*TAttachmentHelpers"Length of output: 234
Script:
#!/bin/bash # Check IssueAttachmentsUploadDetails implementation and its usage of upload status rg -A 10 "IssueAttachmentsUploadDetails.*=.*" web/core/components/issues/attachment/ # Check the helper type definition and its usage rg -A 5 "type TAttachmentHelpers = " web/core/components/issues/ # Check attachment-item-list.tsx content as it uses attachmentHelpers cat web/core/components/issues/attachment/attachment-item-list.tsxLength of output: 7436
13-13: Verify the structure of TAttachmentHelpers type.The type change from specific operation handler to a more generic helpers object is a good abstraction.
Let's verify the type definition:
✅ Verification successful
Let me try a different approach to find the type definition.
Let me check the related types (
TAttachmentOperationsandTAttachmentSnapshot) to fully understand the structure.
I can now generate the final response based on the comprehensive type information found.
Type structure verified and properly supports upload progress tracking
The
TAttachmentHelperstype is well-structured with:
operations: Handles file operations (create/remove)snapshot: Contains upload status tracking viauploadStatusarrayThe type change from
handleAttachmentOperationstoattachmentHelpersis justified as it now encompasses both operations and status tracking capabilities, which aligns with the PR's objective to handle upload progress.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the structure of TAttachmentHelpers type # Expect: Type should include upload status handling capabilities ast-grep --pattern 'type TAttachmentHelpers = { $$$ }'Length of output: 234
Script:
#!/bin/bash # Search for TAttachmentHelpers type definition using ripgrep rg "type TAttachmentHelpers" -A 5 # Also search in potential type definition files fd "types?.ts" --exec rg "TAttachmentHelpers" {} \;Length of output: 666
Script:
#!/bin/bash # Search for related type definitions rg "type TAttachmentOperations" -A 10 rg "type TAttachmentSnapshot" -A 10 # Also search for their usage context rg "TAttachmentOperations|TAttachmentSnapshot" -B 2 -A 2Length of output: 10572
web/core/services/api.service.ts (2)
2-2: LGTM! Important type import for upload progress tracking.The addition of
AxiosRequestConfigtype is crucial as it provides type safety for upload progress configuration through theonUploadProgresscallback.
38-40: Let me try a different search strategy to find attachment upload implementations.#!/bin/bash # Search for attachment-related files and their content echo "Searching for attachment-related files..." rg -l "attachment" --type ts echo -e "\nSearching for file upload implementations..." rg -A 5 "upload.*file|file.*upload" --type ts echo -e "\nSearching for post method usage with FormData..." rg -A 5 "post.*FormData|FormData.*post" --type tsThe previous search had issues with the file type and the pattern might have been too specific. This new search should help us find:
- Files related to attachments
- File upload implementations
- POST requests using FormData (common pattern for file uploads)
web/core/components/issues/attachment/delete-attachment-modal.tsx (2)
11-11: LGTM! Type definition improvementThe change from
ExcludetoPickis a good improvement as it makes the type definition more explicit and follows the principle of least privilege by only selecting the required operation.Also applies to: 13-13
19-19: LGTM! Prop naming improvementThe rename to
attachmentOperationsis more concise and better reflects the prop's purpose. This change maintains type safety while improving readability.web/core/components/issues/attachment/attachment-list-upload-item.tsx (2)
1-17: LGTM! Well-organized imports and clean type definition.The imports are properly categorized and the Props type is clearly defined.
19-45: Verify integration with attachment store.Let's verify that the component correctly observes changes from the MobX store.
✅ Verification successful
Let me gather more information about the store implementation and usage to verify the integration.
Based on the gathered information, I can now generate the final response as we have sufficient evidence about the store integration.
Integration with attachment store is properly implemented
The component correctly integrates with the MobX store:
- Component is wrapped with
observerHOC to react to store changes- Store implements observable
attachmentsUploadStatusMapwith proper typing- Upload status updates are handled through MobX actions that modify the observable map
- Parent component correctly passes the upload status as props
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for attachment store implementation and verify upload status handling ast-grep --pattern 'interface IIssueAttachmentStore { $$$ attachmentsUploadStatusMap: $_; $$$ }' # Search for store usage in parent components rg -A 5 'IssueAttachmentsUploadItem.*uploadStatus'Length of output: 754
Script:
#!/bin/bash # Search for attachment store implementation ast-grep --pattern 'class $_ implements IIssueAttachmentStore { $$$ attachmentsUploadStatusMap = $_; $$$ }' # Search for store initialization and usage rg -A 5 'attachmentsUploadStatusMap' # Search for observer usage in the component rg -B 5 'IssueAttachmentsUploadItem.*observer'Length of output: 4685
web/core/components/issues/attachment/attachment-upload-details.tsx (1)
1-18: LGTM: Well-organized imports and clean type definition.The imports are properly organized with clear grouping, and the Props interface is focused and simple.
web/core/components/issues/attachment/attachment-upload.tsx (2)
7-14: LGTM! Type definitions enhance safety and clarity.The type refinements and prop renaming improve type safety by explicitly restricting operations to "create" only, while making the interface more semantic.
Line range hint
1-64: Verify implementation against PR requirements.The current implementation shows a basic loading state but doesn't fully meet the PR objective of showing upload progress. Please ensure that:
- The attachment upload API supports progress tracking
- Progress information is properly passed through
attachmentOperations.create- UI reflects the actual upload progress
web/core/components/issues/issue-detail-widgets/issue-detail-widget-collapsibles.tsx (2)
28-28: LGTM: Clean hook dependency additionThe addition of
getAttachmentsUploadStatusByIssueIdto the destructured hooks is well-placed and follows the existing pattern.
40-42: LGTM: Improved attachment visibility logicThe new implementation elegantly handles both existing attachments and in-progress uploads. The condition ensures the attachment section remains visible during active uploads, providing better user feedback.
A few key points worth noting:
- The section shows if there are existing attachments (
attachment_count > 0)- The section also shows if there are ongoing uploads (
attachmentUploads.length > 0)- The null checks (
!!) provide good safety against undefined valuesweb/core/components/issues/issue-detail-widgets/attachments/quick-action-button.tsx (2)
33-33: LGTM! Improved hook usage with explicit destructuring.The destructuring of operations from the hook makes the code more maintainable by clearly showing what we're consuming from the hook.
70-70: LGTM! Proper dependency array update.The dependency array correctly includes all necessary values used within the callback.
web/core/services/issue/issue_attachment.service.ts (3)
1-1: LGTM: Appropriate type import for upload progress handlingThe addition of
AxiosRequestConfigimport is correct for typing the upload progress handler.
39-40: LGTM: Well-typed optional parameter additionThe upload progress handler is correctly implemented as an optional parameter with the appropriate axios type, maintaining backward compatibility.
50-54: Verify FileUploadService implementationLet's verify that the FileUploadService's uploadFile method properly implements the upload progress handler parameter.
✅ Verification successful
Let me try a different search strategy to locate the FileUploadService implementation.
This will help us:
- Find files containing FileUploadService class
- Look for uploadFile method implementation with context
The first command helps locate the file, while the second one searches for the method implementation with surrounding context to verify the progress handler parameter usage.
Based on the search results, I can now provide a final response:
FileUploadService properly implements the upload progress handler parameter
The implementation in
web/core/services/file-upload.service.tsshows that theuploadFilemethod correctly accepts and handles theuploadProgressHandlerparameter:async uploadFile( url: string, data: FormData, uploadProgressHandler?: AxiosRequestConfig["onUploadProgress"] ): Promise<void>The method signature matches the usage in
issue_attachment.service.ts, and the parameter is properly typed as an optional Axios upload progress handler. This confirms that the integration is correctly implemented.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the uploadFile method implementation in FileUploadService ast-grep --pattern 'class FileUploadService { $$$ uploadFile($$$) { $$$ } $$$ }'Length of output: 94
Script:
#!/bin/bash # Search for files containing FileUploadService fd -t f . | xargs rg "FileUploadService" -l # Search for uploadFile method implementation rg "uploadFile.*\(" -A 10Length of output: 25945
web/core/components/issues/attachment/attachment-detail.tsx (3)
22-22: LGTM! Import refactor improves code organization.Moving the attachment helpers to a dedicated file improves code organization and maintainability.
26-30: LGTM! Props interface is well-defined.The prop rename to
attachmentHelpersbetter describes its purpose, and the type constraint appropriately restricts available operations.
34-34: LGTM! Component implementation properly adapted.The component correctly implements the new attachment helpers structure while maintaining its original functionality.
Also applies to: 59-59
web/core/components/issues/attachment/attachment-item-list.tsx (3)
11-11: LGTM: Type and import changes align with upload progress feature.The introduction of
TAttachmentHelperstype andIssueAttachmentsUploadItemcomponent properly supports the new upload progress functionality.Also applies to: 14-14, 21-21
26-26: LGTM: Improved state management and prop handling.The renaming of
isLoadingtoisUploadingand the structured destructuring ofattachmentHelpersimprove code clarity and semantic accuracy.Also applies to: 28-28, 35-37
Line range hint
1-122: Verify upload progress implementation meets requirements.The implementation successfully achieves the PR objective of showing upload progress for issue attachments. The code includes:
- Real-time upload status display
- Proper error handling
- User-friendly drag and drop interface
- Disabled state handling
Let's verify the component's integration:
✅ Verification successful
Let me try different commands to verify the implementation.
Implementation is properly integrated and consistent across the codebase
The verification shows that:
- The
attachmentHelpersprop is consistently used throughout the attachment-related components hierarchy:
- Initialized in parent components (
content.tsxandroot.tsx)- Properly passed down through
attachments-list.tsxtoattachment-item-list.tsx- Correctly utilized in child components for operations and snapshot access
- The
IssueAttachmentsUploadItemcomponent is:
- Properly implemented in
attachment-list-upload-item.tsx- Correctly imported and used in
attachment-item-list.tsx- Exported through the barrel file (
index.ts)The implementation demonstrates proper component integration, consistent prop passing, and adherence to the component hierarchy design.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent usage of the new attachmentHelpers prop rg -l "attachmentHelpers" --type typescript # Verify the presence of required components rg -l "IssueAttachmentsUploadItem" --type typescriptLength of output: 166
Script:
#!/bin/bash # Check for consistent usage of the new attachmentHelpers prop rg "attachmentHelpers" # Verify the presence of required components rg "IssueAttachmentsUploadItem" # Look for related attachment upload components and their usage rg "attachment-list-upload-item"Length of output: 2642
web/core/components/issues/issue-detail-widgets/attachments/helper.tsx (1)
105-110: Handle potentialundefinedvalue inattachmentsUploadStatusThe
attachmentsUploadStatusretrieved bygetAttachmentsUploadStatusByIssueId(issueId)may beundefined. Ensure that any components consumingsnapshot.uploadStatushandle theundefinedcase gracefully to prevent runtime errors.Consider providing a default value or adding null checks where
uploadStatusis used.return { operations: attachmentOperations, - snapshot: { uploadStatus: attachmentsUploadStatus }, + snapshot: { uploadStatus: attachmentsUploadStatus || [] }, };
| const fileName = uploadStatus.name; | ||
| const fileExtension = getFileExtension(uploadStatus.name ?? ""); | ||
| const fileIcon = getFileIcon(fileExtension, 18); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checks and memoize derived values.
The file name and extension calculations should be memoized and protected against undefined values.
- const fileName = uploadStatus.name;
- const fileExtension = getFileExtension(uploadStatus.name ?? "");
- const fileIcon = getFileIcon(fileExtension, 18);
+ const fileName = useMemo(() => uploadStatus.name ?? "", [uploadStatus.name]);
+ const fileExtension = useMemo(
+ () => getFileExtension(fileName),
+ [fileName]
+ );
+ const fileIcon = useMemo(
+ () => getFileIcon(fileExtension, 18),
+ [fileExtension]
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fileName = uploadStatus.name; | |
| const fileExtension = getFileExtension(uploadStatus.name ?? ""); | |
| const fileIcon = getFileIcon(fileExtension, 18); | |
| const fileName = useMemo(() => uploadStatus.name ?? "", [uploadStatus.name]); | |
| const fileExtension = useMemo( | |
| () => getFileExtension(fileName), | |
| [fileName] | |
| ); | |
| const fileIcon = useMemo( | |
| () => getFileIcon(fileExtension, 18), | |
| [fileExtension] | |
| ); |
| return ( | ||
| <div className="flex items-center justify-between gap-3 h-11 bg-custom-background-90 pl-9 pr-2 pointer-events-none"> | ||
| <div className="flex items-center gap-3 text-sm truncate"> | ||
| <div className="flex-shrink-0">{fileIcon}</div> | ||
| <Tooltip tooltipContent={fileName} isMobile={isMobile}> | ||
| <p className="text-custom-text-200 font-medium truncate">{fileName}</p> | ||
| </Tooltip> | ||
| </div> | ||
| <div className="flex-shrink-0 flex items-center gap-2"> | ||
| <span className="flex-shrink-0"> | ||
| <CircularProgressIndicator size={20} strokeWidth={3} percentage={uploadStatus.progress} /> | ||
| </span> | ||
| <div className="flex-shrink-0 text-sm font-medium">{uploadStatus.progress}% done</div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Enhance accessibility attributes.
The upload item should have proper ARIA attributes for better screen reader support.
- <div className="flex items-center justify-between gap-3 h-11 bg-custom-background-90 pl-9 pr-2 pointer-events-none">
+ <div
+ role="status"
+ aria-label={`Uploading ${fileName}, ${uploadStatus.progress}% complete`}
+ className="flex items-center justify-between gap-3 h-11 bg-custom-background-90 pl-9 pr-2 pointer-events-none"
+ >📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <div className="flex items-center justify-between gap-3 h-11 bg-custom-background-90 pl-9 pr-2 pointer-events-none"> | |
| <div className="flex items-center gap-3 text-sm truncate"> | |
| <div className="flex-shrink-0">{fileIcon}</div> | |
| <Tooltip tooltipContent={fileName} isMobile={isMobile}> | |
| <p className="text-custom-text-200 font-medium truncate">{fileName}</p> | |
| </Tooltip> | |
| </div> | |
| <div className="flex-shrink-0 flex items-center gap-2"> | |
| <span className="flex-shrink-0"> | |
| <CircularProgressIndicator size={20} strokeWidth={3} percentage={uploadStatus.progress} /> | |
| </span> | |
| <div className="flex-shrink-0 text-sm font-medium">{uploadStatus.progress}% done</div> | |
| </div> | |
| </div> | |
| return ( | |
| <div | |
| role="status" | |
| aria-label={`Uploading ${fileName}, ${uploadStatus.progress}% complete`} | |
| className="flex items-center justify-between gap-3 h-11 bg-custom-background-90 pl-9 pr-2 pointer-events-none" | |
| > | |
| <div className="flex items-center gap-3 text-sm truncate"> | |
| <div className="flex-shrink-0">{fileIcon}</div> | |
| <Tooltip tooltipContent={fileName} isMobile={isMobile}> | |
| <p className="text-custom-text-200 font-medium truncate">{fileName}</p> | |
| </Tooltip> | |
| </div> | |
| <div className="flex-shrink-0 flex items-center gap-2"> | |
| <span className="flex-shrink-0"> | |
| <CircularProgressIndicator size={20} strokeWidth={3} percentage={uploadStatus.progress} /> | |
| </span> | |
| <div className="flex-shrink-0 text-sm font-medium">{uploadStatus.progress}% done</div> | |
| </div> | |
| </div> |
| export const IssueAttachmentsUploadDetails: React.FC<Props> = observer((props) => { | ||
| // props | ||
| const { uploadStatus } = props; | ||
| // derived values | ||
| const fileName = uploadStatus.name; | ||
| const fileExtension = getFileExtension(uploadStatus.name ?? ""); | ||
| const fileIcon = getFileIcon(fileExtension, 28); | ||
| // hooks | ||
| const { isMobile } = usePlatformOS(); | ||
|
|
||
| return ( | ||
| <div className="flex h-[60px] items-center justify-between gap-1 rounded-md border-[2px] border-custom-border-200 bg-custom-background-90 px-4 py-2 text-sm pointer-events-none"> | ||
| <div className="flex-shrink-0 flex items-center gap-3"> | ||
| <div className="h-7 w-7">{fileIcon}</div> | ||
| <div className="flex flex-col gap-1"> | ||
| <div className="flex items-center gap-2"> | ||
| <Tooltip tooltipContent={fileName} isMobile={isMobile}> | ||
| <span className="text-sm">{truncateText(`${fileName}`, 10)}</span> | ||
| </Tooltip> | ||
| </div> | ||
|
|
||
| <div className="flex items-center gap-3 text-xs text-custom-text-200"> | ||
| <span>{fileExtension.toUpperCase()}</span> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <div className="flex-shrink-0 flex items-center gap-2"> | ||
| <span className="flex-shrink-0"> | ||
| <CircularProgressIndicator size={20} strokeWidth={3} percentage={uploadStatus.progress} /> | ||
| </span> | ||
| <div className="flex-shrink-0 text-sm font-medium">{uploadStatus.progress}% done</div> | ||
| </div> | ||
| </div> | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider performance optimizations.
The component could benefit from performance optimizations to prevent unnecessary re-renders.
- Memoize derived values:
+ const derivedValues = useMemo(() => ({
+ fileName: uploadStatus.name || "Unknown file",
+ fileExtension: getFileExtension(uploadStatus.name ?? ""),
+ fileIcon: getFileIcon(fileExtension, 28)
+ }), [uploadStatus.name]);- Extract the progress indicator into a separate memoized component to prevent re-renders of the entire component when only the progress changes.
Committable suggestion was skipped due to low confidence.
web/core/components/issues/issue-detail-widgets/attachments/helper.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
web/core/store/issue/issue-details/attachment.store.ts (3)
17-23: Add JSDoc comments to improve type documentation.Consider adding JSDoc comments to document the purpose and constraints of each field in the
TAttachmentUploadStatustype.+/** + * Represents the upload status of an attachment + * @property id - Temporary UUID for tracking the upload + * @property name - Original filename + * @property progress - Upload progress percentage (0-100) + * @property size - File size in bytes + * @property type - MIME type of the file + */ export type TAttachmentUploadStatus = { id: string; name: string; progress: number; size: number; type: string; };
60-60: Initialize nested map structure in constructor.Consider initializing the nested map structure in the constructor to prevent potential runtime errors when accessing nested properties.
-attachmentsUploadStatusMap: Record<string, Record<string, TAttachmentUploadStatus>> = {}; +attachmentsUploadStatusMap: Record<string, Record<string, TAttachmentUploadStatus>> = {}; + +constructor(rootStore: IIssueRootStore) { + // ... existing code ... + runInAction(() => { + this.attachmentsUploadStatusMap = new Proxy({}, { + get: (target, issueId: string) => target[issueId] || (target[issueId] = {}) + }); + }); +}
96-100: Add type guards and defensive checks.The helper method could be more robust with additional type guards and null checks.
getAttachmentsUploadStatusByIssueId = computedFn((issueId: string) => { - if (!issueId) return undefined; + if (!issueId || typeof issueId !== 'string') return undefined; + if (!(issueId in this.attachmentsUploadStatusMap)) return undefined; const attachmentsUploadStatus = Object.values(this.attachmentsUploadStatusMap[issueId] ?? {}); - return attachmentsUploadStatus ?? undefined; + return attachmentsUploadStatus.length > 0 ? attachmentsUploadStatus : undefined; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- web/core/store/issue/issue-details/attachment.store.ts (6 hunks)
🧰 Additional context used
🪛 Biome
web/core/store/issue/issue-details/attachment.store.ts
[error] 160-160: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- web/core/store/issue/issue-details/attachment.store.ts (6 hunks)
🧰 Additional context used
🪛 Biome
web/core/store/issue/issue-details/attachment.store.ts
[error] 160-160: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
web/core/store/issue/issue-details/attachment.store.ts (3)
17-23: LGTM: Well-structured type definitions for upload status trackingThe new
TAttachmentUploadStatustype and interface updates are well-designed, providing a comprehensive structure for tracking upload progress.Also applies to: 47-47, 51-51
60-60: LGTM: Proper MobX observable setupThe
attachmentsUploadStatusMapis correctly initialized and properly configured as an observable property.Also applies to: 72-72
135-157: 🛠️ Refactor suggestionConsider adding file validation
The method should validate the file before upload (size limits, allowed types, etc.).
createAttachment = async (workspaceSlug: string, projectId: string, issueId: string, file: File) => { const tempId = uuidv4(); + // Validate file + const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5MB + const ALLOWED_TYPES = ['image/jpeg', 'image/png', 'application/pdf']; + + if (file.size > MAX_FILE_SIZE) { + throw new Error(`File size exceeds ${MAX_FILE_SIZE / (1024 * 1024)}MB limit`); + } + if (!ALLOWED_TYPES.includes(file.type)) { + throw new Error(`File type ${file.type} not supported`); + } + try {
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
web/core/components/issues/attachment/attachment-item-list.tsx (1)
96-129: Consider enhancing accessibility for drag and drop.While the UI improvements for upload status are great, consider these accessibility enhancements:
- Add ARIA labels to the drag and drop area
- Ensure keyboard navigation support for the upload functionality
<div {...getRootProps()} + role="button" + aria-label="Upload attachment" className={`relative flex flex-col ${isDragActive && issueAttachments.length < 3 ? "min-h-[200px]" : ""} ${ disabled ? "cursor-not-allowed" : "cursor-pointer" }`} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- web/core/components/issues/attachment/attachment-item-list.tsx (4 hunks)
- web/core/components/issues/issue-detail-widgets/attachments/content.tsx (2 hunks)
- web/core/components/issues/issue-detail-widgets/attachments/helper.tsx (3 hunks)
- web/core/components/issues/issue-detail-widgets/attachments/quick-action-button.tsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/core/components/issues/issue-detail-widgets/attachments/content.tsx
- web/core/components/issues/issue-detail-widgets/attachments/quick-action-button.tsx
🔇 Additional comments (5)
web/core/components/issues/issue-detail-widgets/attachments/helper.tsx (2)
3-22: LGTM! Well-structured type definitions.The new type definitions create a clear separation between operations and status snapshots, making the code more maintainable and type-safe.
Line range hint
23-110: LGTM! Comprehensive implementation with proper error handling and analytics.The hook implementation provides a robust solution for managing attachment operations with:
- Proper loading states and user feedback
- Comprehensive error handling
- Analytics tracking for both success and failure cases
- Efficient memoization with complete dependency array
web/core/components/issues/attachment/attachment-item-list.tsx (3)
10-22: LGTM! Props and types are well-structured.The refactoring of props to use
TAttachmentHelpersimproves type safety and maintainability.
Line range hint
58-84: LGTM! Upload handling logic is sound.The upload logic correctly handles file restrictions and provides user feedback.
27-39: Verify state synchronization.While the state management is cleaner, be cautious of potential race conditions between
isUploadinganduploadStatus. Consider if both states are necessary or ifuploadStatusalone could handle all cases.
| try { | ||
| if (!workspaceSlug || !projectId || !issueId) throw new Error("Missing required fields"); | ||
|
|
||
| const attachmentUploadPromise = createAttachment(workspaceSlug, projectId, issueId, file); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error messages and standardizing event payloads.
- The error message "Missing required fields" could be more specific about which fields are missing.
- The event tracking payload structure differs between create and remove methods - the remove method includes additional update details on failure.
Consider applying these improvements:
- if (!workspaceSlug || !projectId || !issueId) throw new Error("Missing required fields");
+ if (!workspaceSlug || !projectId || !issueId) {
+ const missingFields = [
+ !workspaceSlug && "workspaceSlug",
+ !projectId && "projectId",
+ !issueId && "issueId",
+ ].filter(Boolean);
+ throw new Error(`Missing required fields: ${missingFields.join(", ")}`);
+ }And standardize the event payload structure:
captureIssueEvent({
eventName: "Issue attachment deleted",
payload: { id: issueId, state: "FAILED", element: "Issue detail page" },
- updates: {
- changed_property: "attachment",
- change_details: "",
- },
});Also applies to: 69-73
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- web/core/store/issue/issue-details/attachment.store.ts (6 hunks)
🧰 Additional context used
🪛 Biome
web/core/store/issue/issue-details/attachment.store.ts
[error] 160-160: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
web/core/store/issue/issue-details/attachment.store.ts (1)
17-23: LGTM! Well-structured type definitions.The new
TAttachmentUploadStatustype and interface updates are well-designed, providing comprehensive tracking of upload status with all necessary fields.Also applies to: 47-47, 51-51
| this.rootIssueStore.issues.updateIssue(issueId, { | ||
| attachment_count: issueAttachmentsCount + 1, // increment attachment count | ||
| }); |
There was a problem hiding this comment.
Potential race condition in attachment count update.
The current implementation might lead to incorrect attachment counts if multiple uploads complete simultaneously, as the count is read and updated in separate operations.
- this.rootIssueStore.issues.updateIssue(issueId, {
- attachment_count: issueAttachmentsCount + 1, // increment attachment count
- });
+ // Use atomic increment operation
+ this.rootIssueStore.issues.updateIssue(issueId, (issue) => ({
+ ...issue,
+ attachment_count: (issue.attachment_count || 0) + 1,
+ }));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.rootIssueStore.issues.updateIssue(issueId, { | |
| attachment_count: issueAttachmentsCount + 1, // increment attachment count | |
| }); | |
| // Use atomic increment operation | |
| this.rootIssueStore.issues.updateIssue(issueId, (issue) => ({ | |
| ...issue, | |
| attachment_count: (issue.attachment_count || 0) + 1, | |
| })); |
| debouncedUpdateProgress = debounce((issueId: string, tempId: string, progress: number) => { | ||
| runInAction(() => { | ||
| set(this.attachmentsUploadStatusMap, [issueId, tempId, "progress"], progress); | ||
| }); | ||
| }, 16); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider increasing the debounce interval.
The current debounce interval of 16ms (roughly one frame) might be too aggressive, potentially causing unnecessary re-renders. Consider increasing it to a more reasonable value (e.g., 100-250ms) to balance responsiveness and performance.
debouncedUpdateProgress = debounce((issueId: string, tempId: string, progress: number) => {
runInAction(() => {
set(this.attachmentsUploadStatusMap, [issueId, tempId, "progress"], progress);
});
- }, 16);
+ }, 100); // Increased interval for better performance📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| debouncedUpdateProgress = debounce((issueId: string, tempId: string, progress: number) => { | |
| runInAction(() => { | |
| set(this.attachmentsUploadStatusMap, [issueId, tempId, "progress"], progress); | |
| }); | |
| }, 16); | |
| debouncedUpdateProgress = debounce((issueId: string, tempId: string, progress: number) => { | |
| runInAction(() => { | |
| set(this.attachmentsUploadStatusMap, [issueId, tempId, "progress"], progress); | |
| }); | |
| }, 100); // Increased interval for better performance |
| createAttachment = async (workspaceSlug: string, projectId: string, issueId: string, file: File) => { | ||
| const tempId = uuidv4(); | ||
| try { | ||
| // update attachment upload status | ||
| runInAction(() => { | ||
| update(this.attachments, [issueId], (attachmentIds = []) => uniq(concat(attachmentIds, [response.id]))); | ||
| set(this.attachmentMap, response.id, response); | ||
| this.rootIssueStore.issues.updateIssue(issueId, { | ||
| attachment_count: issueAttachmentsCount + 1, // increment attachment count | ||
| set(this.attachmentsUploadStatusMap, [issueId, tempId], { | ||
| id: tempId, | ||
| name: file.name, | ||
| progress: 0, | ||
| size: file.size, | ||
| type: file.type, | ||
| }); | ||
| }); | ||
| } | ||
| const response = await this.issueAttachmentService.uploadIssueAttachment( | ||
| workspaceSlug, | ||
| projectId, | ||
| issueId, | ||
| file, | ||
| (progressEvent) => { | ||
| const progressPercentage = Math.round((progressEvent.progress ?? 0) * 100); | ||
| this.debouncedUpdateProgress(issueId, tempId, progressPercentage); | ||
| } | ||
| ); |
There was a problem hiding this comment.
💡 Codebase verification
Use existing file size constant and add type validation
The codebase already defines MAX_STATIC_FILE_SIZE in constants/common.ts for file size validation. Let's reuse this constant instead of defining a new one.
+import { MAX_STATIC_FILE_SIZE } from "@/constants/common";
+
createAttachment = async (workspaceSlug: string, projectId: string, issueId: string, file: File) => {
const tempId = uuidv4();
+ const ALLOWED_TYPES = ['image/jpeg', 'image/png', 'application/pdf']; // adjust as needed
+
+ if (file.size > MAX_STATIC_FILE_SIZE) {
+ throw new Error(`File size exceeds maximum limit of ${MAX_STATIC_FILE_SIZE / (1024 * 1024)}MB`);
+ }
+
+ if (!ALLOWED_TYPES.includes(file.type)) {
+ throw new Error(`File type ${file.type} is not supported`);
+ }
+
try {
// update attachment upload status🔗 Analysis chain
Add file validation before upload.
The method should validate the file size and type before initiating the upload to prevent unnecessary network requests and provide better user feedback.
createAttachment = async (workspaceSlug: string, projectId: string, issueId: string, file: File) => {
const tempId = uuidv4();
+ const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5MB
+ const ALLOWED_TYPES = ['image/jpeg', 'image/png', 'application/pdf']; // adjust as needed
+
+ if (file.size > MAX_FILE_SIZE) {
+ throw new Error(`File size exceeds maximum limit of ${MAX_FILE_SIZE / (1024 * 1024)}MB`);
+ }
+
+ if (!ALLOWED_TYPES.includes(file.type)) {
+ throw new Error(`File type ${file.type} is not supported`);
+ }
+
try {
// update attachment upload statusLet me verify if there are any file type restrictions defined in the codebase.
Let me check if there are any existing file validation utilities or services that could be reused.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if file size limits are defined elsewhere
rg -i "file.*size.*limit|max.*file.*size" --type ts
Length of output: 5961
Script:
#!/bin/bash
# Check for file type restrictions or allowed file types
rg -i "allowed.*file.*type|file.*type.*support|mime.*type" --type ts
Length of output: 175
Script:
#!/bin/bash
# Search for file validation related code
rg -i "validate.*file|file.*validation" --type ts -A 5
Length of output: 143
Improvements:
An upload indicator is now displayed while the issue attachment is being uploaded.
Media:
Screen.Recording.2024-10-23.at.17.36.19.mov
Plane issue: WEB-2717
Summary by CodeRabbit
New Features
IssueAttachmentsUploadItemandIssueAttachmentsUploadDetails.Bug Fixes
Refactor
attachmentHelpersstructure.IssueAttachmentRootcomponent for improved reactivity with MobX.Documentation