[WIKI-419] chore: new asset duplicate endpoint added#7172
[WIKI-419] chore: new asset duplicate endpoint added#7172sriramveeraghanta merged 45 commits intopreviewfrom
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a workspace-scoped POST endpoint to duplicate uploaded assets (per-asset throttled), server logic to copy storage and create a new FileAsset, client-side service/store/hook wiring to call it, editor file-handler duplicate support, paste-time duplication handlers, and custom-image duplication lifecycle and UI retry controls. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Editor as Editor UI
participant Store as EditorStore / FileService
participant API as DuplicateAssetEndpoint
participant Throttle as AssetRateThrottle
participant DB as FileAsset DB
participant Storage as ObjectStorage
Editor->>Store: duplicateEditorAsset(assetId, entityType, ...)
Store->>API: POST /assets/v2/workspaces/{slug}/duplicate-assets/{assetId}/ { entity_type, entity_id?, project_id? }
API->>Throttle: get_cache_key(view.kwargs) -> "throttle_asset_{assetId}"
Throttle-->>API: allow/deny
API->>DB: GET original FileAsset(assetId)
API->>Storage: copy original_key -> new_dest_key
API->>DB: CREATE FileAsset (uploaded=true, linked FK)
API-->>Store: 201 { asset_id: new_asset_id }
Store-->>Editor: return { asset_id: new_asset_id }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
web/core/services/file.service.ts (1)
36-45: 🛠️ Refactor suggestionRemove duplicate enum definition.
The local
TFileAssetTypeenum duplicates the importedEFileAssetTypeenum. This creates unnecessary duplication and potential maintenance issues.Since
EFileAssetTypeis now imported from@plane/types, remove the local enum definition and use the imported one consistently throughout the codebase.- export enum TFileAssetType { - COMMENT_DESCRIPTION = "COMMENT_DESCRIPTION", - ISSUE_ATTACHMENT = "ISSUE_ATTACHMENT", - ISSUE_DESCRIPTION = "ISSUE_DESCRIPTION", - PAGE_DESCRIPTION = "PAGE_DESCRIPTION", - PROJECT_COVER = "PROJECT_COVER", - USER_AVATAR = "USER_AVATAR", - USER_COVER = "USER_COVER", - WORKSPACE_LOGO = "WORKSPACE_LOGO", - }
🧹 Nitpick comments (1)
apiserver/plane/app/views/asset/v2.py (1)
783-783: Fix line length violation.The line exceeds the maximum length limit (106 > 88 characters) as flagged by static analysis.
Break the long line for better readability:
- destination_key = f"{workspace.id}/{uuid.uuid4().hex}-{original_asset.attributes.get('name')}" + destination_key = ( + f"{workspace.id}/{uuid.uuid4().hex}-" + f"{original_asset.attributes.get('name')}" + )🧰 Tools
🪛 Ruff (0.11.9)
783-783: Line too long (106 > 88)
(E501)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apiserver/plane/app/urls/asset.py(2 hunks)apiserver/plane/app/views/__init__.py(1 hunks)apiserver/plane/app/views/asset/v2.py(1 hunks)web/core/services/file.service.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apiserver/plane/app/urls/asset.py (1)
apiserver/plane/app/views/asset/v2.py (1)
DuplicateAssetEndpoint(723-810)
🪛 Ruff (0.11.9)
apiserver/plane/app/views/asset/v2.py
783-783: Line too long (106 > 88)
(E501)
🪛 Pylint (3.3.7)
apiserver/plane/app/views/asset/v2.py
[refactor] 724-724: Too many return statements (7/6)
(R0911)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
apiserver/plane/app/views/__init__.py (1)
110-110: LGTM!The import addition is correctly placed and follows the existing pattern for asset v2 endpoints.
apiserver/plane/app/urls/asset.py (1)
16-16: LGTM!The import addition follows the existing pattern and is correctly placed.
web/core/services/file.service.ts (2)
4-4: Good import addition.The import of
EFileAssetTypefrom the types package is the correct approach for type consistency.
262-276: LGTM!The
duplicateAssetsmethod is well-implemented with:
- Correct parameter types and return type
- Proper endpoint URL construction
- Consistent error handling pattern
- Clear method signature that matches the backend API
apiserver/plane/app/views/asset/v2.py (1)
804-808: LGTM!The bulk update to set
is_uploaded=Truefor all duplicated assets is efficient and correct. The conditional check ensures the update only happens when there are successfully duplicated assets.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apiserver/plane/app/views/asset/v2.py (4)
728-759: 🛠️ Refactor suggestionExtract duplicated method to reduce code duplication.
This method is duplicated across multiple endpoint classes (
WorkspaceFileAssetEndpoint,ProjectAssetEndpoint, and nowDuplicateAssetEndpoint), violating the DRY principle.Extract this method to the
BaseAPIViewclass or create a mixin. Additionally, refactor to use a dictionary mapping to reduce the number of return statements:def get_entity_id_field(self, entity_type, entity_id): entity_field_mapping = { FileAsset.EntityTypeContext.WORKSPACE_LOGO: {"workspace_id": entity_id}, FileAsset.EntityTypeContext.PROJECT_COVER: {"project_id": entity_id}, FileAsset.EntityTypeContext.USER_AVATAR: {"user_id": entity_id}, FileAsset.EntityTypeContext.USER_COVER: {"user_id": entity_id}, FileAsset.EntityTypeContext.ISSUE_ATTACHMENT: {"issue_id": entity_id}, FileAsset.EntityTypeContext.ISSUE_DESCRIPTION: {"issue_id": entity_id}, FileAsset.EntityTypeContext.PAGE_DESCRIPTION: {"page_id": entity_id}, FileAsset.EntityTypeContext.COMMENT_DESCRIPTION: {"comment_id": entity_id}, } return entity_field_mapping.get(entity_type, {})🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 728-728: Too many return statements (7/6)
(R0911)
761-767: 🛠️ Refactor suggestionAdd validation for entity_type parameter.
The
entity_typeparameter is retrieved but not validated against allowed values, which could lead to invalid data being stored.Add validation after line 766:
entity_type = request.data.get("entity_type", None) + + if entity_type and entity_type not in FileAsset.EntityTypeContext.values: + return Response( + {"error": "Invalid entity type.", "status": False}, + status=status.HTTP_400_BAD_REQUEST, + )
783-785: 🛠️ Refactor suggestionAdd validation for asset existence and upload status.
The code fetches assets but doesn't validate that all requested
asset_idsexist or that the assets are uploaded, which could lead to silent failures.Add validation after fetching the assets:
original_assets = FileAsset.objects.filter( workspace=workspace, id__in=asset_ids ) + + # Validate all requested assets exist + found_asset_ids = set(str(asset.id) for asset in original_assets) + missing_asset_ids = set(asset_ids) - found_asset_ids + if missing_asset_ids: + return Response( + {"error": f"Assets not found: {', '.join(missing_asset_ids)}"}, + status=status.HTTP_404_NOT_FOUND, + ) + + # Validate all assets are uploaded + unuploaded_assets = [str(asset.id) for asset in original_assets if not asset.is_uploaded] + if unuploaded_assets: + return Response( + {"error": f"Assets not uploaded: {', '.join(unuploaded_assets)}"}, + status=status.HTTP_400_BAD_REQUEST, + )
805-806:⚠️ Potential issueAdd error handling for storage copy operation.
The
storage.copy_object()call lacks error handling, which could leave the database in an inconsistent state if the copy fails.Wrap the storage operation in error handling:
- storage.copy_object(original_asset.asset, destination_key) - duplicated_assets[str(original_asset.id)] = str(duplicated_asset.id) + try: + storage.copy_object(original_asset.asset, destination_key) + duplicated_assets[str(original_asset.id)] = str(duplicated_asset.id) + except Exception as e: + # Clean up the created asset record if storage copy fails + duplicated_asset.delete() + return Response( + {"error": f"Failed to copy storage object: {str(e)}"}, + status=status.HTTP_500_INTERNAL_SERVER_ERROR, + )
🧹 Nitpick comments (1)
apiserver/plane/app/views/asset/v2.py (1)
808-814: Consider transaction safety for bulk operations.The bulk update operation and the preceding asset creation loop could benefit from database transaction wrapping to ensure atomicity.
Consider wrapping the duplication logic in a database transaction:
+ from django.db import transaction + + @transaction.atomic def post(self, request, slug):This ensures that if any part of the duplication process fails, all changes are rolled back consistently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apiserver/plane/app/views/asset/v2.py(2 hunks)apiserver/plane/settings/common.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
apiserver/plane/app/views/asset/v2.py
787-787: Line too long (106 > 88)
(E501)
🪛 Pylint (3.3.7)
apiserver/plane/app/views/asset/v2.py
[refactor] 728-728: Too many return statements (7/6)
(R0911)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
apiserver/plane/settings/common.py (1)
74-78: LGTM! Throttling configuration is appropriate.The throttling settings are well-configured for the new duplicate asset endpoint:
AnonRateThrottleprovides rate limiting for anonymous users- The
image_duplicatescope with 10 requests/minute is reasonable for asset duplication operations- The anonymous rate of 30 requests/minute is balanced
apiserver/plane/app/views/asset/v2.py (2)
14-14: Import addition looks good.The
ScopedRateThrottleimport is necessary for the new duplicate endpoint's throttling functionality.
723-727: Throttling configuration is properly implemented.The class-level throttling configuration using
ScopedRateThrottlewith the"image_duplicate"scope addresses the rate limiting requirement mentioned in past reviews.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apiserver/plane/app/views/asset/v2.py (4)
727-758: 🛠️ Refactor suggestionExtract duplicated method to base class.
The
get_entity_id_fieldmethod is duplicated across multiple endpoint classes in this file (WorkspaceFileAssetEndpoint,ProjectAssetEndpoint, and nowDuplicateAssetEndpoint). This violates the DRY principle and creates maintenance overhead.Consider extracting this method to the
BaseAPIViewclass or creating a mixin class that can be shared across these asset endpoints.Additionally, the static analysis correctly identifies that this method has too many return statements (7/6), which affects readability. Consider using a dictionary lookup pattern:
def get_entity_id_field(self, entity_type, entity_id): entity_field_mapping = { FileAsset.EntityTypeContext.WORKSPACE_LOGO: {"workspace_id": entity_id}, FileAsset.EntityTypeContext.PROJECT_COVER: {"project_id": entity_id}, FileAsset.EntityTypeContext.USER_AVATAR: {"user_id": entity_id}, FileAsset.EntityTypeContext.USER_COVER: {"user_id": entity_id}, FileAsset.EntityTypeContext.ISSUE_ATTACHMENT: {"issue_id": entity_id}, FileAsset.EntityTypeContext.ISSUE_DESCRIPTION: {"issue_id": entity_id}, FileAsset.EntityTypeContext.PAGE_DESCRIPTION: {"page_id": entity_id}, FileAsset.EntityTypeContext.COMMENT_DESCRIPTION: {"comment_id": entity_id}, } return entity_field_mapping.get(entity_type, {})🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 727-727: Too many return statements (7/6)
(R0911)
764-764: 🛠️ Refactor suggestionAdd validation for entity_type when provided.
The
entity_typeparameter is accepted but not validated against allowed values. This could lead to invalid entity types being stored in the database.Add validation similar to other endpoints in this file:
+ if entity_type and entity_type not in FileAsset.EntityTypeContext.values: + return Response( + {"error": "Invalid entity type.", "status": False}, + status=status.HTTP_400_BAD_REQUEST, + )
775-777:⚠️ Potential issueAdd validation for asset existence and upload status.
The code fetches the original asset but doesn't validate that the requested
asset_idexists or that the asset is uploaded. This could lead to silent failures or attempting to copy non-existent storage objects.Add validation after fetching the asset:
original_asset = FileAsset.objects.filter( workspace=workspace, id=asset_id ).first() + + # Validate asset exists + if not original_asset: + return Response( + {"error": "Asset not found"}, + status=status.HTTP_404_NOT_FOUND, + ) + + # Validate asset is uploaded + if not original_asset.is_uploaded: + return Response( + {"error": "Asset not uploaded"}, + status=status.HTTP_400_BAD_REQUEST, + )
795-795:⚠️ Potential issueAdd error handling for storage copy operation.
The
storage.copy_object()call can fail if the source object doesn't exist or due to storage service issues, but there's no error handling. This could leave the database in an inconsistent state with asset records that don't have corresponding storage objects.Add error handling around the storage operation:
- storage.copy_object(original_asset.asset, destination_key) + try: + storage.copy_object(original_asset.asset, destination_key) + except Exception as e: + # Clean up the created asset record if storage copy fails + duplicated_asset.delete() + return Response( + {"error": f"Failed to copy storage object: {str(e)}"}, + status=status.HTTP_500_INTERNAL_SERVER_ERROR, + )
🧹 Nitpick comments (1)
apiserver/plane/app/views/asset/v2.py (1)
779-779: Fix line length violation.The line exceeds the maximum length of 88 characters.
- destination_key = f"{workspace.id}/{uuid.uuid4().hex}-{original_asset.attributes.get('name')}" + destination_key = ( + f"{workspace.id}/{uuid.uuid4().hex}-" + f"{original_asset.attributes.get('name')}" + )🧰 Tools
🪛 Ruff (0.11.9)
779-779: Line too long (102 > 88)
(E501)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apiserver/plane/app/throttles/asset.py(1 hunks)apiserver/plane/app/urls/asset.py(2 hunks)apiserver/plane/app/views/asset/v2.py(2 hunks)apiserver/plane/settings/common.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apiserver/plane/app/urls/asset.py
- apiserver/plane/settings/common.py
🧰 Additional context used
🪛 Pylint (3.3.7)
apiserver/plane/app/throttles/asset.py
[refactor] 4-4: Too few public methods (1/2)
(R0903)
apiserver/plane/app/views/asset/v2.py
[refactor] 727-727: Too many return statements (7/6)
(R0911)
🪛 Ruff (0.11.9)
apiserver/plane/app/views/asset/v2.py
779-779: Line too long (102 > 88)
(E501)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
apiserver/plane/app/throttles/asset.py (1)
1-11: LGTM! Clean throttling implementation.The
AssetRateThrottleclass correctly implements rate limiting based onasset_id. The implementation follows Django REST framework patterns and properly handles cases whereasset_idmight not be present.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 4-4: Too few public methods (1/2)
(R0903)
apiserver/plane/app/views/asset/v2.py (2)
22-22: LGTM! Proper import of throttling class.The import correctly adds the
AssetRateThrottleclass needed for the new endpoint.
723-726: LGTM! Throttling properly configured.The endpoint correctly applies the
AssetRateThrottleto implement rate limiting based on asset ID as requested in previous reviews.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
apiserver/plane/app/views/asset/v2.py (2)
795-797:storage.copy_objectstill lacks error handlingA previous review pointed this out. Failure here leaves a DB row without a real object.
Wrap intry/except, roll back the newFileAsset, and return a 500.
727-758: 🛠️ Refactor suggestion
get_entity_id_fieldis still duplicated – extract or centralise to stay DRYThis helper is now present in three endpoint classes with identical logic. The earlier review already requested moving it into a shared location (e.g., a mix-in or
BaseAPIView) and replacing the cascade ofifstatements with a lookup dict.
Keeping three copies will inevitably drift.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 727-727: Too many return statements (7/6)
(R0911)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apiserver/plane/app/urls/asset.py(2 hunks)apiserver/plane/app/views/__init__.py(1 hunks)apiserver/plane/app/views/asset/v2.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apiserver/plane/app/views/init.py
- apiserver/plane/app/urls/asset.py
🧰 Additional context used
🪛 Ruff (0.11.9)
apiserver/plane/app/views/asset/v2.py
779-779: Line too long (102 > 88)
(E501)
🪛 Pylint (3.3.7)
apiserver/plane/app/views/asset/v2.py
[refactor] 727-727: Too many return statements (7/6)
(R0911)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
iam-vipin
dismissed stale reviews from dheeru0198 and Palanikannan1437
via
c0ebe35
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/editor/src/core/extensions/custom-image/components/block.tsx(3 hunks)packages/editor/src/core/extensions/custom-image/components/node-view.tsx(5 hunks)packages/editor/src/core/extensions/custom-image/components/uploader.tsx(8 hunks)packages/editor/src/core/extensions/custom-image/extension.tsx(3 hunks)packages/editor/src/core/extensions/custom-image/utils.ts(3 hunks)packages/editor/src/core/extensions/utility.ts(2 hunks)packages/editor/src/core/props.ts(0 hunks)packages/editor/src/core/types/config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/editor/src/core/props.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/src/core/extensions/custom-image/utils.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-09T20:42:31.843Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19
Timestamp: 2025-10-09T20:42:31.843Z
Learning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.
Applied to files:
packages/editor/src/core/extensions/custom-image/components/uploader.tsx
📚 Learning: 2025-10-01T15:30:17.605Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7888
File: packages/propel/src/avatar/avatar.stories.tsx:2-3
Timestamp: 2025-10-01T15:30:17.605Z
Learning: In the makeplane/plane repository, avoid suggesting inline type imports (e.g., `import { Avatar, type TAvatarSize }`) due to bundler compatibility issues. Keep type imports and value imports as separate statements.
Applied to files:
packages/editor/src/core/extensions/custom-image/components/uploader.tsx
🧬 Code graph analysis (5)
packages/editor/src/core/extensions/utility.ts (1)
packages/editor/src/core/plugins/paste-asset.ts (1)
PasteAssetPlugin(4-48)
packages/editor/src/core/extensions/custom-image/components/uploader.tsx (1)
packages/editor/src/core/extensions/custom-image/components/node-view.tsx (1)
CustomImageNodeViewProps(10-16)
packages/editor/src/core/extensions/custom-image/components/block.tsx (1)
packages/editor/src/core/extensions/custom-image/utils.ts (1)
isImageDuplicating(58-58)
packages/editor/src/core/extensions/custom-image/extension.tsx (1)
apps/web/core/services/page/project-page.service.ts (1)
duplicate(171-177)
packages/editor/src/core/extensions/custom-image/components/node-view.tsx (1)
packages/editor/src/core/extensions/custom-image/utils.ts (1)
hasImageDuplicationFailed(63-64)
🪛 GitHub Actions: Build and lint web apps
packages/editor/src/core/extensions/custom-image/components/node-view.tsx
[error] 5-5: ESLint: Imports 'CustomImageExtensionType' and 'TCustomImageAttributes' are only used as type. (consistent-type-imports)
🪛 GitHub Check: Build and lint web apps
packages/editor/src/core/extensions/custom-image/extension.tsx
[failure] 14-14:
Imports "CustomImageExtensionOptions" and "CustomImageExtensionStorage" are only used as type
packages/editor/src/core/extensions/custom-image/components/node-view.tsx
[failure] 5-5:
Imports "CustomImageExtensionType" and "TCustomImageAttributes" are only used as type
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (11)
packages/editor/src/core/extensions/utility.ts (1)
12-12: LGTM! Clean integration of the paste asset duplication plugin.The import and registration of
PasteAssetPluginare properly implemented. The plugin is positioned at the end of the ProseMirror plugins array and includes guards (data-uploadedcheck,hasChangescondition) to prevent conflicts with other paste handlers likeMarkdownClipboardPlugin.Also applies to: 81-81
packages/editor/src/core/extensions/custom-image/extension.tsx (2)
36-46: LGTM!The duplication option is properly extracted from the file handler and added to the extension options, following the same pattern as the upload functionality.
101-104: LGTM!Using symbolic attribute names (
ECustomImageAttributeNames.IDandECustomImageAttributeNames.STATUS) improves code maintainability and reduces the risk of typos.packages/editor/src/core/extensions/custom-image/components/block.tsx (2)
7-7: LGTM!Import of the duplication helper is appropriate for the new status checks.
206-215: LGTM!The visibility logic correctly handles the duplication state by showing the loader and hiding interactive controls (toolbar, resizer, upload status) during duplication, which provides a clean user experience.
packages/editor/src/core/extensions/custom-image/components/node-view.tsx (2)
30-31: LGTM!Using refs to guard retry attempts and prevent concurrent duplication calls is the correct approach, as these don't need to trigger re-renders.
116-135: LGTM!The rendering logic correctly shows the uploader (with retry UI) when duplication fails and hides the image block, providing a clear path for the user to retry the operation.
packages/editor/src/core/extensions/custom-image/components/uploader.tsx (4)
49-89: LGTM!Status is correctly updated to
UPLOADEDon successful upload, and the upload flow properly manages the image lifecycle.
91-97: LGTM!The upload command now correctly sets the status to
UPLOADINGbefore performing the upload, andupdateAttributesis properly included in the dependency array.
170-195: LGTM!Error handling correctly treats duplication failures as error states, and the retry handler properly initiates a new duplication attempt by updating the status to
DUPLICATING.
197-223: LGTM!The UI correctly styles duplication failures with error states, prevents file picker interaction during failures, and uses proper layout classes for consistent rendering.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/editor/src/core/extensions/custom-image/components/uploader.tsx (1)
169-184: Consider more specific error messages.The function returns a generic "Error loading image" message for both
failedToLoadImageandhasDuplicationFailedcases. Users would benefit from more specific messages to understand whether the issue is with loading the image or duplicating an existing asset.Consider this refinement:
const getDisplayMessage = useCallback(() => { const isUploading = isImageBeingUploaded; - if (failedToLoadImage || hasDuplicationFailed) { - return "Error loading image"; + if (failedToLoadImage) { + return "Failed to load image"; + } + if (hasDuplicationFailed) { + return "Failed to duplicate asset"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/editor/src/core/extensions/custom-image/components/uploader.tsx(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-09T20:42:31.843Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19
Timestamp: 2025-10-09T20:42:31.843Z
Learning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.
Applied to files:
packages/editor/src/core/extensions/custom-image/components/uploader.tsx
📚 Learning: 2025-10-01T15:30:17.605Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7888
File: packages/propel/src/avatar/avatar.stories.tsx:2-3
Timestamp: 2025-10-01T15:30:17.605Z
Learning: In the makeplane/plane repository, avoid suggesting inline type imports (e.g., `import { Avatar, type TAvatarSize }`) due to bundler compatibility issues. Keep type imports and value imports as separate statements.
Applied to files:
packages/editor/src/core/extensions/custom-image/components/uploader.tsx
🧬 Code graph analysis (1)
packages/editor/src/core/extensions/custom-image/components/uploader.tsx (1)
packages/editor/src/core/extensions/custom-image/components/node-view.tsx (1)
CustomImageNodeViewProps(10-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (8)
packages/editor/src/core/extensions/custom-image/components/uploader.tsx (8)
1-1: LGTM: Import additions are correct and necessary.The
RotateCcwicon andECustomImageStatusenum are properly imported and used throughout the component for the new retry functionality.Also applies to: 14-14
20-20: LGTM: New prop properly typed and destructured.The
hasDuplicationFailedprop follows the existing pattern and is correctly integrated into the component.Also applies to: 38-38
54-57: LGTM: Status update on successful upload.Correctly sets the status to
UPLOADEDafter a successful upload, maintaining proper state transitions.
90-96: LGTM: Upload status properly set before upload operation.The function correctly sets the status to
UPLOADINGbefore initiating the upload, andupdateAttributesis properly included in the dependency array. This addresses the past review comment about moving this logic touploadImageEditorCommand.
198-209: LGTM: Error styling properly applied for duplication failures.The red error styling is consistently applied across all states (default, hover, selected) when either
failedToLoadImageorhasDuplicationFailedis true, providing clear visual feedback to users.
215-219: LGTM: File picker properly disabled during duplication failure.Correctly prevents opening the file picker when
hasDuplicationFailedis true, guiding users to use the retry button instead of uploading a new file.
222-238: LGTM: Retry button implementation is complete and addresses past feedback.The retry button is properly implemented with:
- Correct conditional rendering (checks
hasDuplicationFailedinstead of hardcodedtrue)type="button"attribute to prevent form submission- Proper accessibility with title attribute
- Consistent styling and clear icon + text affordance
- Layout enhancement with
flex-1on the message elementThis successfully addresses all past review comments regarding the hardcoded condition and missing button type.
186-194: Status change correctly triggers duplication in parent component.Verification confirms that the parent component (
node-view.tsx) contains auseEffectat line 68 that watches for theDUPLICATINGstatus. WhenhandleRetryClicksets the status toDUPLICATING, the effect triggers and invokes the actual duplication logic viahandleDuplication(). The dependency array correctly includesstatusto ensure the effect re-runs when the status changes.
…e error handling in image duplication
iam-vipin
dismissed stale reviews from Palanikannan1437 and dheeru0198
via
bb43f2f
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/core/components/editor/rich-text/description-input/root.tsx (1)
118-118: Duplicate asset flow wiring looks good; consider logging errors for parityHooking
duplicateEditorAssetfromuseEditorAssetand exposing it asduplicateFileonRichTextEditor(returningasset_idand surfacing a user-friendly error message) cleanly mirrors the existing upload flow and matches the new backend endpoint shape.For easier debugging, you might also log the caught error in
duplicateFile(similar to the upload path) before throwing the user-facing"Asset duplication failed. Please try again later.", e.g. viaconsole.error.Also applies to: 225-255
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/core/components/editor/document/editor.tsx(2 hunks)apps/web/core/components/editor/lite-text/editor.tsx(2 hunks)apps/web/core/components/editor/rich-text/description-input/root.tsx(2 hunks)apps/web/core/components/editor/rich-text/editor.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/web/core/components/editor/document/editor.tsx (1)
packages/editor/src/core/types/config.ts (1)
TFileHandler(5-22)
apps/web/core/components/editor/rich-text/editor.tsx (1)
packages/editor/src/core/types/config.ts (1)
TFileHandler(5-22)
apps/web/core/components/editor/lite-text/editor.tsx (1)
packages/editor/src/core/types/config.ts (1)
TFileHandler(5-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
apps/web/core/components/editor/document/editor.tsx (1)
28-32: duplicateFile wiring into DocumentEditor looks consistent
duplicateFileis correctly added to the editable props surface and passed through togetEditorFileHandlerswith a no-op fallback wheneditableis false, mirroring the existinguploadFilepattern. This keeps the file handling API consistent across editors and aligns withTFileHandler["duplicate"].Also applies to: 66-70
apps/web/core/components/editor/rich-text/editor.tsx (1)
28-32: RichTextEditor duplicateFile plumbing is correct and aligned with other editorsThe new
duplicateFile: TFileHandler["duplicate"]prop foreditable: trueand its forwarding togetEditorFileHandlersusingeditable ? props.duplicateFile : async () => ""is consistent withuploadFileand the sharedTFileHandlershape.Also applies to: 64-68
apps/web/core/components/editor/lite-text/editor.tsx (1)
45-48: LiteTextEditor duplicateFile integration matches shared file-handler contract
duplicateFileis properly added to the editable-only props and forwarded intogetEditorFileHandlerswith a no-op fallback when not editable, matching the establisheduploadFilepattern and the other editor components.Also applies to: 122-126
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/editor/src/core/extensions/custom-image/components/node-view.tsx (1)
82-82: MovehasRetriedOnMountflag to prevent blocking automatic retry.Setting
hasRetriedOnMount.current = truebefore attempting duplication prevents the automatic retry logic (lines 103-109) from ever triggering, even on the first failure. When duplication fails, the status becomesDUPLICATION_FAILED, but the retry effect checks!hasRetriedOnMount.currentwhich evaluates tofalse, so no retry happens.Move this line to after successful duplication:
isDuplicatingRef.current = true; try { - hasRetriedOnMount.current = true; - const newAssetId = await extension.options.duplicateImage!(imgNodeSrc); // Update node with new source and success status updateAttributes({ src: newAssetId, status: ECustomImageStatus.UPLOADED, }); + hasRetriedOnMount.current = true; } catch (error: unknown) {This allows the retry effect to run once after initial failure while preventing further retries after success.
🧹 Nitpick comments (1)
packages/editor/src/core/extensions/custom-image/components/node-view.tsx (1)
103-109: Remove misleading comment about delay.The comment mentions "Add a small delay before retrying" but no delay is actually implemented. The retry happens immediately through the status update.
if (hasImageDuplicationFailed(status) && !hasRetriedOnMount.current && imgNodeSrc) { hasRetriedOnMount.current = true; - // Add a small delay before retrying to avoid immediate retries updateAttributes({ status: ECustomImageStatus.DUPLICATING }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/editor/src/core/extensions/custom-image/components/node-view.tsx(5 hunks)packages/editor/src/core/extensions/custom-image/extension.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/src/core/extensions/custom-image/extension.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T15:30:17.605Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7888
File: packages/propel/src/avatar/avatar.stories.tsx:2-3
Timestamp: 2025-10-01T15:30:17.605Z
Learning: In the makeplane/plane repository, avoid suggesting inline type imports (e.g., `import { Avatar, type TAvatarSize }`) due to bundler compatibility issues. Keep type imports and value imports as separate statements.
Applied to files:
packages/editor/src/core/extensions/custom-image/components/node-view.tsx
🧬 Code graph analysis (1)
packages/editor/src/core/extensions/custom-image/components/node-view.tsx (1)
packages/editor/src/core/extensions/custom-image/utils.ts (1)
hasImageDuplicationFailed(63-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
packages/editor/src/core/extensions/custom-image/components/node-view.tsx (6)
5-7: LGTM! Import structure is correct.The type-only and runtime imports are properly separated, complying with linting rules and bundler compatibility requirements.
20-21: LGTM! Props correctly destructured for duplication logic.The additions of
updateAttributesandstatusare necessary and correctly integrated with the new duplication flow.
31-32: LGTM! Refs appropriately used for state tracking.The refs correctly prevent unnecessary re-renders while tracking duplication state and preventing concurrent calls.
111-115: LGTM! Reset logic is correct.Resetting
hasRetriedOnMountwhen the upload succeeds allows proper retry behavior for future duplication attempts.
117-118: LGTM! Derived values correctly computed.Both
hasDuplicationFailedandshouldShowBlockare properly derived from the current state and used appropriately in rendering logic.
123-136: LGTM! Conditional rendering handles duplication states correctly.The rendering logic properly switches between image block and uploader based on duplication status, and passes the necessary state to the uploader for appropriate UI feedback.
…de-view.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/space/helpers/editor.helper.ts (1)
61-64: Clarify semantics ofduplicatefallback (no-op vs explicit failure)Right now
duplicateresolves with the sameassetId, effectively making duplication a silent no-op for sites/space. If upstream code expects a new asset ID or relies on a rejected promise to show the “Asset duplication failed. Please try again later.” message, this may be misleading.Consider explicitly failing instead, so the UI can surface a clear error when duplication isn’t supported in this app:
- duplicate: async (assetId: string) => - // Duplication is not supported for sites/space app - // Return the same assetId as a fallback - assetId, + duplicate: async (assetId: string) => { + // Duplication is not supported for sites/space app + // Explicitly fail so callers can show an appropriate error + throw new Error("Asset duplication is not supported for sites/space app"); + },If callers are intentionally designed to treat “same ID” as “no-op duplicate” here, please confirm that this is documented and won’t break assumptions elsewhere.
packages/editor/src/core/extensions/custom-image/components/node-view.tsx (1)
68-105: Duplication logic is sound, but consider documenting the retry behavior.The duplication handler correctly validates inputs, prevents race conditions, and handles errors. However, setting
hasRetriedOnMount.current = trueat line 82 (before attempting duplication) prevents the automatic retry effect (lines 107-113) from running after failures that occur during this mount.This appears intentional—automatic retry only triggers for nodes already in
DUPLICATION_FAILEDstate on mount, not for failures during the current session. Consider adding a comment explaining this behavior to prevent confusion.Example clarifying comment:
isDuplicatingRef.current = true; try { + // Set flag before duplication to prevent automatic retry effect after failures in this session. + // Automatic retry only applies to nodes that mount in DUPLICATION_FAILED state. hasRetriedOnMount.current = true; const newAssetId = await extension.options.duplicateImage!(imgNodeSrc);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/space/helpers/editor.helper.ts(1 hunks)packages/editor/src/ce/helpers/asset-duplication.ts(1 hunks)packages/editor/src/core/extensions/custom-image/components/node-view.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/src/ce/helpers/asset-duplication.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T15:30:17.605Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7888
File: packages/propel/src/avatar/avatar.stories.tsx:2-3
Timestamp: 2025-10-01T15:30:17.605Z
Learning: In the makeplane/plane repository, avoid suggesting inline type imports (e.g., `import { Avatar, type TAvatarSize }`) due to bundler compatibility issues. Keep type imports and value imports as separate statements.
Applied to files:
packages/editor/src/core/extensions/custom-image/components/node-view.tsx
🧬 Code graph analysis (1)
packages/editor/src/core/extensions/custom-image/components/node-view.tsx (1)
packages/editor/src/core/extensions/custom-image/utils.ts (1)
hasImageDuplicationFailed(63-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
packages/editor/src/core/extensions/custom-image/components/node-view.tsx (3)
5-7: LGTM!The imports are correctly structured with type-only imports separated from runtime imports, complying with bundler compatibility requirements.
11-32: LGTM!Props typing and state management are well-structured. The refs appropriately prevent race conditions (via
isDuplicatingRef) and track retry state (viahasRetriedOnMount).
115-146: LGTM!The reset logic correctly clears the retry flag on successful upload, and the rendering logic appropriately shows either the image block or the uploader based on duplication status. The
hasDuplicationFailedflag is properly propagated to the uploader component.
* chore: new asset duplicate endpoint added * chore: change the type in url * chore: added rate limiting for image duplication endpoint * chore: added rate limiting per asset id * chore: added throttle class * chore: added validations for entity * chore: added extra validations * chore: removed the comment * chore: reverted the frontend code * chore: added the response key * feat: handle image duplication for web * feat: custom image duplication update * fix: remove paste logic for image * fix : remove entity validation * refactor: remove entity id for duplication * feat: handle duplication in utils * feat: add asset duplication registry * chore: update the set attribute method * fix: add ref for api check * chore :remove logs * chore : add entity types types * refactor: rename duplication success status value * chore: update attribute to enums * chore: update variable name * chore: set uploading state * chore : update enum name * chore : update replace command * chore: fix retry UI * chore: remove default logic * refactor: optimize imports in custom image extension files and improve error handling in image duplication * fix:type error * Update packages/editor/src/core/extensions/custom-image/components/node-view.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: enhance asset duplication handler to ignore HTTP sources --------- Co-authored-by: NarayanBavisetti <narayan3119@gmail.com> Co-authored-by: Bavisetti Narayan <72156168+NarayanBavisetti@users.noreply.github.com> Co-authored-by: VipinDevelops <vipinchaudhary1809@gmail.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* chore: new asset duplicate endpoint added * chore: change the type in url * chore: added rate limiting for image duplication endpoint * chore: added rate limiting per asset id * chore: added throttle class * chore: added validations for entity * chore: added extra validations * chore: removed the comment * chore: reverted the frontend code * chore: added the response key * feat: handle image duplication for web * feat: custom image duplication update * fix: remove paste logic for image * fix : remove entity validation * refactor: remove entity id for duplication * feat: handle duplication in utils * feat: add asset duplication registry * chore: update the set attribute method * fix: add ref for api check * chore :remove logs * chore : add entity types types * refactor: rename duplication success status value * chore: update attribute to enums * chore: update variable name * chore: set uploading state * chore : update enum name * chore : update replace command * chore: fix retry UI * chore: remove default logic * refactor: optimize imports in custom image extension files and improve error handling in image duplication * fix:type error * Update packages/editor/src/core/extensions/custom-image/components/node-view.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: enhance asset duplication handler to ignore HTTP sources --------- Co-authored-by: NarayanBavisetti <narayan3119@gmail.com> Co-authored-by: Bavisetti Narayan <72156168+NarayanBavisetti@users.noreply.github.com> Co-authored-by: VipinDevelops <vipinchaudhary1809@gmail.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* chore: new asset duplicate endpoint added * chore: change the type in url * chore: added rate limiting for image duplication endpoint * chore: added rate limiting per asset id * chore: added throttle class * chore: added validations for entity * chore: added extra validations * chore: removed the comment * chore: reverted the frontend code * chore: added the response key * feat: handle image duplication for web * feat: custom image duplication update * fix: remove paste logic for image * fix : remove entity validation * refactor: remove entity id for duplication * feat: handle duplication in utils * feat: add asset duplication registry * chore: update the set attribute method * fix: add ref for api check * chore :remove logs * chore : add entity types types * refactor: rename duplication success status value * chore: update attribute to enums * chore: update variable name * chore: set uploading state * chore : update enum name * chore : update replace command * chore: fix retry UI * chore: remove default logic * refactor: optimize imports in custom image extension files and improve error handling in image duplication * fix:type error * Update packages/editor/src/core/extensions/custom-image/components/node-view.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: enhance asset duplication handler to ignore HTTP sources --------- Co-authored-by: NarayanBavisetti <narayan3119@gmail.com> Co-authored-by: Bavisetti Narayan <72156168+NarayanBavisetti@users.noreply.github.com> Co-authored-by: VipinDevelops <vipinchaudhary1809@gmail.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Description
This PR introduces a new endpoint to duplicate assets and attach them to an entiyt without the need to re-upload them.
Type of Change
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.