[PE-95] chore: gif image support for all editors #6219
Conversation
WalkthroughThis pull request introduces enhancements to asset management across multiple files, focusing on expanding file type support to include GIF images. The changes span backend API views, frontend configuration, and validation utilities. The modifications allow GIF uploads for user avatars, workspace logos, project covers, and other asset types while maintaining existing validation and handling logic. The updates centralize file type configurations and ensure consistent validation across different components of the application. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 2
🧹 Nitpick comments (2)
packages/editor/src/core/plugins/image/utils/validate-file.ts (1)
17-18: Consider making the error message dynamic based on accepted types.While the validation logic is correct, the error message is hardcoded. Consider deriving it from
ACCEPTED_FILE_MIME_TYPESto ensure it stays in sync with supported formats.Here's a suggested improvement:
if (!ACCEPTED_FILE_MIME_TYPES.includes(file.type)) { - alert("Invalid file type. Please select a JPEG, JPG, PNG, WEBP or GIF file."); + const supportedFormats = ACCEPTED_FILE_MIME_TYPES + .map(type => type.split('/')[1].toUpperCase()) + .join(', '); + alert(`Invalid file type. Please select a ${supportedFormats} file.`); return false; }apiserver/plane/app/views/asset/v2.py (1)
129-135: Consider security implications of GIF file support.While adding GIF support enhances functionality, please ensure:
- Frontend sanitizes GIF files before rendering to prevent XSS attacks
- Consider implementing file content validation to detect malicious GIF payloads
- Consider adding a maximum file dimension limit for GIF files to prevent DOS attacks
Would you like me to provide implementation examples for these security measures?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apiserver/plane/app/views/asset/v2.py(1 hunks)apiserver/plane/space/views/asset.py(1 hunks)packages/editor/src/core/constants/config.ts(1 hunks)packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx(2 hunks)packages/editor/src/core/plugins/image/utils/validate-file.ts(2 hunks)
🔇 Additional comments (3)
packages/editor/src/core/constants/config.ts (1)
9-10: LGTM! Clean implementation of file type configuration.
The centralized configuration of MIME types and dynamic generation of file extensions is well-structured and maintainable. The implementation follows the DRY principle and makes it easy to add or modify supported file types in the future.
packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx (1)
5-6: LGTM! Verify GIF upload functionality end-to-end.
The implementation correctly uses the centralized file extensions configuration for the file input. The component's existing upload handling should work seamlessly with GIF files as it's format-agnostic.
Let's verify the GIF support implementation across the codebase:
Also applies to: 171-171
✅ Verification successful
Let me gather more specific information about the file extensions and MIME types configuration.
Based on the gathered information, I can now provide a final response:
GIF support is properly implemented across the codebase
The implementation correctly supports GIF files:
- Frontend config defines GIF in
ACCEPTED_FILE_MIME_TYPESarray as "image/gif" and automatically maps it to file extension - Backend settings include "image/gif" in
ATTACHMENT_MIME_TYPESlist - Asset upload endpoint explicitly allows GIF files in the allowed types:
"image/gif"is included in theallowed_typeslist
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify GIF support implementation across the codebase
# Check if GIF support is consistently implemented across relevant files
echo "Checking for GIF support consistency..."
# Search for MIME type definitions and file type validations
rg -i "image/(gif|jpeg|jpg|png|webp)" --type ts --type tsx --type py
# Search for file extension references
rg -i "\.(gif|jpeg|jpg|png|webp)" --type ts --type tsx --type py
# Search for potential file type validation in backend
rg -i "allowed_types|accepted.*types|mime.*types" --type py
Length of output: 1076
Script:
#!/bin/bash
# Check the centralized config for accepted file extensions
echo "Checking ACCEPTED_FILE_EXTENSIONS in config..."
fd "config.ts" --type f --exec cat {}
# Check ATTACHMENT_MIME_TYPES configuration
echo -e "\nChecking ATTACHMENT_MIME_TYPES in settings..."
cat apiserver/plane/settings/common.py
# Check asset upload view implementations
echo -e "\nChecking asset upload implementations..."
cat apiserver/plane/space/views/asset.py
Length of output: 38520
apiserver/plane/app/views/asset/v2.py (1)
129-135: Verify S3Storage compatibility with GIF files.
While the code changes look good, we should verify that the S3Storage implementation properly handles GIF files, especially for generating presigned URLs and handling metadata.
✅ Verification successful
Let me check the complete S3Storage implementation to ensure there's no file type specific handling that might affect GIF files.
Let me try a different approach to check the S3Storage implementation.
S3Storage implementation is compatible with GIF files
The S3Storage implementation is file type agnostic and handles all file types uniformly:
- Content-Type is passed through as-is in
generate_presigned_post - No special handling or restrictions for any file types in URL generation
- File metadata operations are type-independent
- Both presigned URLs and posts properly preserve the mime type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check S3Storage implementation for GIF handling
# Search for S3Storage implementation and verify file type handling
rg -A 10 "def generate_presigned"
rg -A 10 "def generate_presigned_post"
Length of output: 2389
Script:
#!/bin/bash
# Get the complete S3Storage class implementation
ast-grep --pattern 'class S3Storage {
$$$
}'
# Also check for any GIF-specific handling in the storage module
rg -i "gif" apiserver/plane/settings/storage.py
Length of output: 96
Script:
#!/bin/bash
# Get the complete content of the storage file
cat apiserver/plane/settings/storage.py
# Also check if there are any file type specific handlers in the codebase
rg -i "content[_-]type.*gif"
Length of output: 5682
| allowed_types = [ | ||
| "image/jpeg", | ||
| "image/png", | ||
| "image/webp", | ||
| "image/jpg", | ||
| "image/gif", | ||
| ] |
There was a problem hiding this comment.
Update error message to reflect all allowed image types.
The error message only mentions JPEG and PNG files, but the allowed_types list includes WEBP, JPG, and GIF. This inconsistency could confuse users.
Apply this diff to update the error message:
if type not in allowed_types:
return Response(
{
- "error": "Invalid file type. Only JPEG and PNG files are allowed.",
+ "error": "Invalid file type. Only JPEG, PNG, WEBP, JPG, and GIF files are allowed.",
"status": False,
},
status=status.HTTP_400_BAD_REQUEST,
)Committable suggestion skipped: line range outside the PR's diff.
| allowed_types = [ | ||
| "image/jpeg", | ||
| "image/png", | ||
| "image/webp", | ||
| "image/jpg", | ||
| "image/gif", | ||
| ] |
There was a problem hiding this comment.
Update error message in UserAssetsV2Endpoint to reflect all allowed image types.
The error message only mentions JPEG and PNG files, but the allowed_types list includes WEBP, JPG, and GIF.
Apply this diff to update the error message:
if type not in allowed_types:
return Response(
{
- "error": "Invalid file type. Only JPEG and PNG files are allowed.",
+ "error": "Invalid file type. Only JPEG, PNG, WEBP, JPG, and GIF files are allowed.",
"status": False,
},
status=status.HTTP_400_BAD_REQUEST,
)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
apiserver/plane/space/views/asset.py (2)
Line range hint
99-104: Update error message to reflect all allowed file types.The error message only mentions JPEG and PNG files, but the allowed types now include WEBP, JPG, and GIF formats.
Here's the suggested fix:
- "error": "Invalid file type. Only JPEG and PNG files are allowed.", + "error": "Invalid file type. Only JPEG, JPG, PNG, WEBP, and GIF files are allowed.",
Centralization of file type configuration is indeed needed
The search results confirm that there are multiple locations where the same file type validations are duplicated:
- In
apiserver/plane/space/views/asset.py- Multiple times in
apiserver/plane/app/views/asset/v2.py- In
packages/editor/src/core/constants/config.ts- A more comprehensive list exists in
apiserver/plane/settings/common.pySpecific issues:
- Duplicate
allowed_typesdefinitions in multiple asset views- The error message mentions only "JPEG and PNG" but the list includes more formats
- The common settings file has a more complete list that should be used as the source of truth
🔗 Analysis chain
Line range hint
89-104: Consider centralizing file type configurations.Since this file type validation is likely needed across multiple components (as indicated by the AI summary mentioning similar changes in other files), consider:
- Moving the allowed types to a central configuration
- Creating a shared validation utility
This would ensure consistency and make future updates easier to maintain.
Let's check for similar file type validations across the codebase:
The previous command failed because the
--type pythonflag wasn't supported. Let's try searching without file type filtering and also look for allowed_types definitions to find similar validations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for file type validations rg -A 2 "image/(jpeg|png|gif|webp)" --type pythonLength of output: 82
Script:
#!/bin/bash # Search for file type validations using ripgrep rg -A 2 "image/(jpeg|png|gif|webp|jpg)" # Also search for allowed_types definitions rg -A 5 "allowed_types.*=.*\["Length of output: 9663
🧹 Nitpick comments (3)
packages/editor/src/core/constants/config.ts (1)
9-10: LGTM! Consider adding type safety.The implementation is clean and maintainable. The MIME types are standard and correct, and the extension derivation is elegant.
Consider adding type safety with a const assertion or explicit type:
-export const ACCEPTED_FILE_MIME_TYPES = ["image/jpeg", "image/jpg", "image/png", "image/webp", "image/gif"]; +export const ACCEPTED_FILE_MIME_TYPES = ["image/jpeg", "image/jpg", "image/png", "image/webp", "image/gif"] as const;packages/editor/src/core/plugins/image/utils/validate-file.ts (1)
17-18: LGTM! Consider i18n for error messages.The validation logic correctly uses the centralized MIME types constant and provides clear error messages.
Consider extracting the error message to an i18n system for better internationalization support:
- alert("Invalid file type. Please select a JPEG, JPG, PNG, WEBP or GIF file."); + alert(t('errors.invalid_file_type', { types: 'JPEG, JPG, PNG, WEBP or GIF' }));packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx (1)
171-171: LGTM! Consider memoizing the joined extensions.The implementation correctly uses the centralized file extensions for the input accept attribute.
Consider memoizing the joined extensions to avoid unnecessary string operations on re-renders:
+const acceptedExtensions = useMemo(() => ACCEPTED_FILE_EXTENSIONS.join(','), []); // ... - accept={ACCEPTED_FILE_EXTENSIONS.join(",")} + accept={acceptedExtensions}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apiserver/plane/app/views/asset/v2.py(1 hunks)apiserver/plane/space/views/asset.py(1 hunks)packages/editor/src/core/constants/config.ts(1 hunks)packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx(2 hunks)packages/editor/src/core/plugins/image/utils/validate-file.ts(2 hunks)
🔇 Additional comments (2)
apiserver/plane/app/views/asset/v2.py (1)
129-135: Security measures for file uploads look good
The implementation includes proper security measures:
- File size validation with configurable limits
- MIME type validation
- Secure storage using S3Storage
- Proper permission checks
apiserver/plane/space/views/asset.py (1)
89-95: LGTM on GIF support addition!
The addition of GIF and JPG formats to the allowed types list aligns with the PR objective of adding GIF support across editors.
| allowed_types = [ | ||
| "image/jpeg", | ||
| "image/png", | ||
| "image/webp", | ||
| "image/jpg", | ||
| "image/gif", | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Centralize file type configuration and update error messages
- The allowed file types list is duplicated across multiple classes (UserAssetsV2Endpoint, WorkspaceFileAssetEndpoint, ProjectAssetEndpoint).
- Error messages are inconsistent with the actual allowed types.
Consider these improvements:
- Create a central configuration for allowed file types:
# Add to a new file: plane/utils/constants.py
class FileAssetConfig:
ALLOWED_MIME_TYPES = [
"image/jpeg",
"image/png",
"image/webp",
"image/jpg",
"image/gif",
]
@classmethod
def get_human_readable_types(cls):
return "JPEG, PNG, WebP, and GIF"- Update the endpoints to use the central configuration:
+from plane.utils.constants import FileAssetConfig
class UserAssetsV2Endpoint(BaseAPIView):
def post(self, request):
# ...
- allowed_types = [
- "image/jpeg",
- "image/png",
- "image/webp",
- "image/jpg",
- "image/gif",
- ]
- if type not in allowed_types:
+ if type not in FileAssetConfig.ALLOWED_MIME_TYPES:
return Response(
{
- "error": "Invalid file type. Only JPEG and PNG files are allowed.",
+ "error": f"Invalid file type. Only {FileAssetConfig.get_human_readable_types()} files are allowed.",
"status": False,
},
status=status.HTTP_400_BAD_REQUEST,
)Apply similar changes to WorkspaceFileAssetEndpoint and ProjectAssetEndpoint classes.
aaryan610
changed the title
Description
This PR adds gif support for all editors.
Type of Change
Summary by CodeRabbit
New Features
Bug Fixes
Chores