[PE-242, 243] refactor: editor file handling, image upload status #6442
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of file handling and upload mechanisms across multiple components in the editor system. The changes primarily focus on enhancing type safety, introducing a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Editor
participant AssetStore
participant FileService
User->>Editor: Upload File
Editor->>AssetStore: Track Upload (blockId)
AssetStore->>FileService: Upload Asset
FileService-->>AssetStore: Upload Progress
AssetStore->>AssetStore: Update Upload Status
FileService-->>Editor: Upload Complete
Editor->>User: File Uploaded
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
✨ Finishing Touches
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 (
|
aaryan610
changed the title
aaryan610
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (2)
web/core/components/issues/issue-detail/issue-activity/root.tsx (1)
Line range hint
94-100: Improve error handling with specific error messages.The error handling has been oversimplified. Consider capturing and handling specific error types to provide more meaningful feedback to users.
- } catch { + } catch (error) { + console.error("Error creating comment:", error); + const message = error instanceof Error ? error.message : "Comment creation failed. Please try again later."; setToast({ title: "Error!", type: TOAST_TYPE.ERROR, - message: "Comment creation failed. Please try again later.", + message, }); }Also applies to: 111-117, 128-134
packages/editor/src/core/hooks/use-file-upload.ts (1)
Line range hint
66-66: Add missing dependencies to useCallback.The dependency array is missing several dependencies that are used within the callback:
editor,blockId,maxFileSize, andloadImageFromFileSystem.Apply this fix:
}, - [onUpload] + [editor, blockId, maxFileSize, loadImageFromFileSystem, onUpload] );
🧹 Nitpick comments (19)
web/core/components/editor/lite-text-editor/lite-text-editor.tsx (1)
64-65: Consider adding a comment explaining the editor config hook's purpose.The implementation correctly uses the new
useEditorConfighook to manage file handlers. Consider adding a brief comment explaining its role in the refactored file handling system.// editor config +// Provides file handlers with workspace and project context for the editor const { getEditorFileHandlers } = useEditorConfig();web/core/components/inbox/modals/create-modal/issue-description.tsx (1)
86-97: Consider enhancing error handling and type safety.While the upload functionality implementation is good, there are a few areas for improvement:
- Error handling could be more informative and structured
- The
data.idusage should be handled more safely as it's optionalConsider this improvement:
uploadFile={async (blockId, file) => { try { const { asset_id } = await uploadEditorAsset({ blockId, data: { - entity_identifier: data.id ?? "", + entity_identifier: data.id || `temp_${Date.now()}`, // Provide a temporary ID if data.id is not available entity_type: EFileAssetType.ISSUE_DESCRIPTION, }, file, projectId, workspaceSlug, }); onAssetUpload?.(asset_id); return asset_id; } catch (error) { - console.log("Error in uploading issue asset:", error); - throw new Error("Asset upload failed. Please try again later."); + console.error("Error uploading issue asset:", { error, blockId, projectId }); + throw new Error(`Asset upload failed: ${error instanceof Error ? error.message : 'Unknown error'}`); } }}web/core/components/editor/rich-text-editor/rich-text-editor.tsx (1)
21-21: Consider adding JSDoc comments for the uploadFile property.The type change to
TFileHandler["upload"]improves type safety. Consider adding JSDoc comments to document the expected behavior and parameters of the upload function.+ /** Function to handle file uploads in the editor */ uploadFile: TFileHandler["upload"];web/core/components/issues/issue-modal/components/description-editor.tsx (1)
199-212: Enhance error handling and loading state management.While the upload implementation is good, consider these improvements:
- Provide more specific error messages based on error types
- Add loading state management to disable the editor during upload
uploadFile={async (blockId, file) => { + const [isUploading, setIsUploading] = useState(false); try { + setIsUploading(true); const { asset_id } = await uploadEditorAsset({ blockId, data: { entity_identifier: issueId ?? "", entity_type: isDraft ? EFileAssetType.DRAFT_ISSUE_DESCRIPTION : EFileAssetType.ISSUE_DESCRIPTION, }, file, projectId, workspaceSlug, }); onAssetUpload(asset_id); return asset_id; } catch (error) { - console.log("Error in uploading issue asset:", error); - throw new Error("Asset upload failed. Please try again later."); + if (error.status === 413) { + throw new Error("File size too large. Please upload a smaller file."); + } else if (error.status === 415) { + throw new Error("File type not supported."); + } else { + console.error("Error in uploading issue asset:", error); + throw new Error("Asset upload failed. Please try again later."); + } } finally { + setIsUploading(false); } }}web/core/store/editor/asset.store.ts (2)
72-76: Re-evaluate the debounce interval for progress updatesThe
debouncedUpdateProgressfunction uses a debounce interval of 16 milliseconds, which corresponds to approximately 60fps. Consider if such a high update frequency is necessary for upload progress indicators. A longer interval (e.g., 100ms) might reduce unnecessary renders and improve performance without significantly affecting the user experience.
80-91: Assess the necessity oftempIdin upload statusThe
tempIdgenerated byuuidv4()is assigned to theidfield inassetsUploadStatusbut doesn't appear to be used elsewhere in the code. If theidis not required for further operations, consider removing it to simplify the status object.Apply this diff if you choose to remove
tempId:- const tempId = uuidv4(); ... - set(this.assetsUploadStatus, [blockId], { - id: tempId, + set(this.assetsUploadStatus, [blockId], { name: file.name, progress: 0, size: file.size, type: file.type, });packages/editor/src/core/extensions/custom-image/components/upload-status.tsx (1)
17-21: Enhance accessibility and error handling.The upload status indicator needs improvements in the following areas:
- Add ARIA attributes for screen readers
- Handle edge cases like upload failures
- Consider adding a loading state
Consider applying these changes:
return ( <div className="absolute top-1 right-1 z-20 bg-black/60 rounded text-xs font-medium w-10 text-center" + role="progressbar" + aria-valuenow={uploadStatus} + aria-valuemin="0" + aria-valuemax="100" + aria-label="Image upload progress" > - {uploadStatus}% + {uploadStatus !== undefined ? `${uploadStatus}%` : 'Loading...'} </div> );packages/editor/src/core/types/config.ts (1)
Line range hint
8-17: Consider enhancing type documentation.The type structure looks good, but could benefit from additional JSDoc comments explaining the purpose of each property.
Consider adding documentation:
export type TFileHandler = TReadOnlyFileHandler & { + /** Maps block IDs to their upload progress (0-100) */ assetsUploadStatus: Record<string, number>; + /** Cancels ongoing upload operations */ cancel: () => void; + /** Handles asset deletion */ delete: DeleteImage; + /** Handles asset upload */ upload: UploadImage; validation: { /** * @description max file size in bytes * @example enter 5242880( 5* 1024 * 1024) for 5MB */ maxFileSize: number; }; };space/core/components/editor/lite-text-read-only-editor.tsx (1)
15-15: LGTM! Consider making workspaceId required.The addition of
workspaceIdand its integration withgetReadOnlyEditorFileHandlersis well-implemented. The changes align with the PR objectives of refactoring file handling.Consider marking
workspaceIdas required in the type definition by moving it outside the intersection:type LiteTextReadOnlyEditorWrapperProps = { workspaceId: string; anchor: string; } & Omit< ILiteTextReadOnlyEditor, "disabledExtensions" | "fileHandler" | "mentionHandler" >;Also applies to: 19-26
space/core/components/editor/rich-text-editor.tsx (1)
3-3: LGTM! Consider adding JSDoc comments.The changes improve type safety by using
TFileHandler["upload"]and properly integrate the newworkspaceIdandanchorprops. The implementation aligns well with the PR objectives.Consider adding JSDoc comments to document the new props:
interface RichTextEditorWrapperProps extends Omit<IRichTextEditor, "disabledExtensions" | "fileHandler" | "mentionHandler"> { /** Unique identifier for the editor instance */ anchor: string; /** Function to handle file uploads */ uploadFile: TFileHandler["upload"]; /** ID of the workspace context */ workspaceId: string; }Also applies to: 11-13, 17-17, 27-29
space/core/components/issues/peek-overview/issue-details.tsx (1)
16-18: Consider using consistent casing for workspaceID.The variable uses uppercase "ID" while the prop uses lowercase "Id". While functionally correct, maintaining consistent casing would improve code readability.
- const { project_details, workspace: workspaceID } = usePublish(anchor); + const { project_details, workspace: workspaceId } = usePublish(anchor);packages/editor/src/core/hooks/use-read-only-editor.ts (1)
14-14: LGTM! Type changes enhance type safety for read-only operations.The switch to
TReadOnlyFileHandlerproperly enforces read-only behavior at the type system level.Consider adding JSDoc comments to document the read-only nature of the
fileHandlerproperty:+ /** File handler with read-only operations */ fileHandler: TReadOnlyFileHandler;Also applies to: 23-23
web/core/components/issues/issue-detail/issue-activity/comments/comment-create.tsx (1)
113-117: LGTM! File upload handler correctly implements the new signature.The changes properly integrate with the new file handling mechanism while maintaining state management.
Consider extracting the file upload handler type for better type safety:
type FileUploadHandler = (blockId: string, file: File) => Promise<string>;packages/editor/src/core/types/editor.ts (1)
19-19: LGTM! Well-structured type definitions.The addition of
TReadOnlyFileHandlerand attachment-related types enhances type safety and provides proper typing for the new attachment functionality. The changes are consistent with TypeScript best practices.Consider documenting the new types with JSDoc comments for better developer experience.
+/** Handler for read-only file operations */ TReadOnlyFileHandler, +/** Commands supported by the editor */ export type TEditorCommands = +/** Extra properties required for specific commands */ export type TCommandExtraProps =Also applies to: 48-49, 55-57, 162-162
web/core/components/issues/description-input.tsx (1)
131-142: Enhance error handling in uploadFile.While the implementation looks good, the error handling could be improved:
- The error message could be more specific based on the error type
- Consider adding error tracking/logging
} catch (error) { - console.log("Error in uploading issue asset:", error); - throw new Error("Asset upload failed. Please try again later."); + console.error("Error uploading issue asset:", error); + // Track error for monitoring + if (error instanceof TypeError) { + throw new Error("Invalid file format. Please try again with a supported file type."); + } else { + throw new Error("Asset upload failed. Please try again later."); + } }packages/editor/src/core/extensions/custom-image/custom-image.ts (1)
36-36: Consider initializing assetsUploadStatus with an empty object.The
assetsUploadStatusis added to storage but its initial value is undefined. Consider initializing it with an empty object to prevent potential undefined access issues.addStorage() { return { fileMap: new Map(), deletedImageSet: new Map<string, boolean>(), uploadInProgress: false, maxFileSize, markdown: { serialize() {}, }, - assetsUploadStatus, + assetsUploadStatus: assetsUploadStatus ?? {}, }; }Also applies to: 133-133
packages/editor/src/core/hooks/use-editor.ts (1)
130-133: Add a safety check for editor commands.Consider adding a safety check for the editor commands to prevent potential issues if they're not initialized:
if (!editor) return; + if (!editor.commands?.updateAssetsUploadStatus) return; const assetsUploadStatus = fileHandler.assetsUploadStatus; editor.commands.updateAssetsUploadStatus(assetsUploadStatus);packages/editor/src/core/extensions/custom-image/components/image-block.tsx (1)
214-215: Consider a more comprehensive condition for upload status visibility.The current condition might not cover all cases where upload status should be shown.
- const showUploadStatus = !resolvedImageSrc; + const showUploadStatus = !resolvedImageSrc && !hasErroredOnFirstLoad && !hasTriedRestoringImageOnce;This ensures that upload status is not shown when:
- The image has errored on first load
- We've already tried restoring the image
packages/editor/src/core/hooks/use-file-upload.ts (1)
Line range hint
156-187: Consider enhancing error handling and validation.The error handling in
uploadFirstImageAndInsertRemainingcould be improved:
- Console warnings/errors should be propagated to the UI for better user feedback.
- File validation could be more robust (e.g., checking file types using magic numbers).
Would you like me to provide a more detailed implementation for these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (53)
packages/editor/src/core/components/editors/document/read-only-editor.tsx(2 hunks)packages/editor/src/core/extensions/custom-image/components/image-block.tsx(3 hunks)packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx(1 hunks)packages/editor/src/core/extensions/custom-image/components/upload-status.tsx(1 hunks)packages/editor/src/core/extensions/custom-image/custom-image.ts(5 hunks)packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts(2 hunks)packages/editor/src/core/extensions/extensions.tsx(1 hunks)packages/editor/src/core/extensions/image/read-only-image.tsx(1 hunks)packages/editor/src/core/extensions/read-only-extensions.tsx(2 hunks)packages/editor/src/core/hooks/use-editor.ts(1 hunks)packages/editor/src/core/hooks/use-file-upload.ts(2 hunks)packages/editor/src/core/hooks/use-read-only-editor.ts(2 hunks)packages/editor/src/core/types/collaboration.ts(2 hunks)packages/editor/src/core/types/config.ts(1 hunks)packages/editor/src/core/types/editor.ts(3 hunks)packages/editor/src/core/types/image.ts(1 hunks)space/core/components/editor/lite-text-editor.tsx(2 hunks)space/core/components/editor/lite-text-read-only-editor.tsx(1 hunks)space/core/components/editor/rich-text-editor.tsx(2 hunks)space/core/components/editor/rich-text-read-only-editor.tsx(1 hunks)space/core/components/issues/peek-overview/comment/add-comment.tsx(1 hunks)space/core/components/issues/peek-overview/comment/comment-detail-card.tsx(2 hunks)space/core/components/issues/peek-overview/issue-details.tsx(2 hunks)space/helpers/editor.helper.ts(3 hunks)web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx(6 hunks)web/ce/components/pages/editor/ai/ask-pi-menu.tsx(3 hunks)web/ce/components/pages/editor/ai/menu.tsx(3 hunks)web/core/components/core/modals/gpt-assistant-popover.tsx(6 hunks)web/core/components/editor/lite-text-editor/lite-text-editor.tsx(3 hunks)web/core/components/editor/lite-text-editor/lite-text-read-only-editor.tsx(1 hunks)web/core/components/editor/rich-text-editor/rich-text-editor.tsx(2 hunks)web/core/components/editor/rich-text-editor/rich-text-read-only-editor.tsx(1 hunks)web/core/components/editor/sticky-editor/editor.tsx(3 hunks)web/core/components/inbox/modals/create-modal/issue-description.tsx(3 hunks)web/core/components/issues/description-input.tsx(4 hunks)web/core/components/issues/issue-detail/issue-activity/comments/comment-card.tsx(2 hunks)web/core/components/issues/issue-detail/issue-activity/comments/comment-create.tsx(1 hunks)web/core/components/issues/issue-detail/issue-activity/root.tsx(7 hunks)web/core/components/issues/issue-modal/components/description-editor.tsx(4 hunks)web/core/components/pages/editor/editor-body.tsx(5 hunks)web/core/components/pages/editor/page-root.tsx(3 hunks)web/core/components/pages/version/editor.tsx(3 hunks)web/core/components/profile/activity/activity-list.tsx(3 hunks)web/core/components/profile/activity/profile-activity-list.tsx(1 hunks)web/core/hooks/editor/index.ts(1 hunks)web/core/hooks/editor/use-editor-config.ts(1 hunks)web/core/hooks/store/index.ts(1 hunks)web/core/hooks/store/use-editor-asset.ts(1 hunks)web/core/services/file.service.ts(5 hunks)web/core/store/editor/asset.store.ts(1 hunks)web/core/store/issue/issue-details/attachment.store.ts(1 hunks)web/core/store/root.store.ts(4 hunks)web/helpers/editor.helper.ts(0 hunks)
💤 Files with no reviewable changes (1)
- web/helpers/editor.helper.ts
✅ Files skipped from review due to trivial changes (2)
- web/core/hooks/editor/index.ts
- packages/editor/src/core/extensions/extensions.tsx
🔇 Additional comments (68)
web/core/components/editor/lite-text-editor/lite-text-editor.tsx (3)
5-5: LGTM! Import changes align with file handling refactor.The addition of
TFileHandlertype anduseEditorConfighook imports support the PR's objective of improving file handling mechanisms.Also applies to: 12-12
30-30: LGTM! Type-safe interface update.The change to use
TFileHandler["upload"]improves type safety and aligns with the separation of editable and read-only file handlers.
Line range hint
1-120: Great implementation of the file handling refactor!The changes successfully:
- Improve type safety with
TFileHandler- Properly integrate the new editor configuration system
- Maintain clean code structure and React best practices
The implementation aligns perfectly with the PR objectives of refactoring the editor's file handling mechanism.
web/core/components/editor/sticky-editor/editor.tsx (4)
5-5: LGTM! Import changes align with type safety improvements.The addition of
TFileHandlertype import supports the PR's objective of improving file handler type definitions.
11-11: LGTM! Hook-based approach improves editor configuration management.The switch to
useEditorConfighook aligns with React best practices and provides better integration with the component lifecycle.
28-28: LGTM! Type definition improvement enhances maintainability.Using
TFileHandler["upload"]instead of an inline function type ensures consistency with the centralized type system and makes future updates easier to manage.
52-53: LGTM! Hook usage properly implements editor configuration.The
useEditorConfighook is correctly implemented, and the destructuredgetEditorFileHandlersfunction is appropriately used within the component.web/core/components/inbox/modals/create-modal/issue-description.tsx (2)
20-20: LGTM! Import changes align with the refactoring objectives.The addition of
useEditorAssethook import is consistent with the PR's goal of refactoring the file handling mechanism.
51-52: LGTM! Hook implementation is clean and well-placed.The
uploadEditorAssethook is correctly extracted and placed with other store hooks.space/core/components/issues/peek-overview/comment/comment-detail-card.tsx (2)
143-143: LGTM! Consistent workspace ID handling.The addition of the
workspaceIdprop toLiteTextReadOnlyEditormaintains consistency with the editable editor component.
115-118: LGTM! Verify other usages ofuploadFile.The updated
uploadFilesignature correctly implements the new file handling mechanism.Let's verify that all other components using
uploadFilehave been updated to include theblockIdparameter:✅ Verification successful
All usages of
uploadFileproperly implement theblockIdparameterThe codebase consistently implements the updated
uploadFilesignature withblockIdparameter across all components usingLiteTextEditor. The type system ensures compliance through theTFileHandlerinterface.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for uploadFile prop usage in components ast-grep --pattern 'uploadFile={$_}' .Length of output: 40
Script:
#!/bin/bash # Search for LiteTextEditor component usage with context rg "LiteTextEditor" -A 5 -B 5 # Backup search for uploadFile occurrences rg "uploadFile" -A 2 -B 2Length of output: 47447
web/core/components/editor/rich-text-editor/rich-text-editor.tsx (2)
3-3: LGTM! Import changes align with file handling refactor.The addition of
TFileHandlertype anduseEditorConfighook supports better type safety and modular configuration management.Also applies to: 11-11
Line range hint
33-44: Verify that all required parameters are properly propagated to child components.The implementation correctly uses the
useEditorConfighook and passes all necessary parameters togetEditorFileHandlers. This change improves modularity and configuration management.Let's verify that all required parameters are properly typed and used in child components:
✅ Verification successful
Parameters are properly propagated and type-safe across all components
The verification confirms:
- Consistent parameter passing across all editor components
- Proper type definitions and inheritance (TFileHandler extends TReadOnlyFileHandler)
- Correct usage of read-only vs. writable handlers based on component needs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for getEditorFileHandlers usage to ensure consistent parameter passing ast-grep --pattern 'getEditorFileHandlers({ $$$ })' # Search for TFileHandler usage to verify type consistency rg -t typescript "TFileHandler" -A 2Length of output: 4918
Script:
#!/bin/bash # Search for type definitions and useEditorConfig implementation rg "type.*FileHandler" -g "*.ts*" -A 2 # Find useEditorConfig implementation ast-grep --pattern 'export function useEditorConfig() { $$$ }' # Alternative search for useEditorConfig rg "useEditorConfig" -g "*.ts*" -A 5Length of output: 10733
web/core/components/pages/editor/page-root.tsx (1)
7-7: LGTM! Clean integration of webhook connection parameters.The changes properly integrate webhook connection parameters throughout the component hierarchy, maintaining consistency in the data flow.
Also applies to: 39-39, 44-44, 120-120
web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx (1)
116-122: LGTM! Well-structured webhook configuration.The webhook connection parameters are properly memoized and include all necessary fields.
web/core/components/pages/editor/editor-body.tsx (1)
133-142: LGTM! Clean realtime configuration setup.The realtime configuration is properly set up with webhook parameters and includes good error handling.
web/core/components/issues/issue-detail/issue-activity/root.tsx (1)
136-154: LGTM! Well-structured asset upload handling.The uploadCommentAsset function properly handles file uploads with the new blockId parameter and includes appropriate error handling.
web/core/components/issues/issue-modal/components/description-editor.tsx (2)
77-77: LGTM! Good use of custom hook for asset management.The introduction of
useEditorAssethook aligns well with the PR's objective of refactoring file handling and provides a centralized way to manage editor assets.
30-31: Consider moving service instantiation inside the component.Moving service instantiations outside the component could cause issues if these services require runtime configuration. Consider moving them inside the component or using a dependency injection pattern.
Let's check if these services require runtime configuration:
web/core/store/editor/asset.store.ts (1)
85-91: Confirm all state mutations occur within actionsWhile you are using
runInActionto wrap state mutations, double-check that all modifications toassetsUploadStatusare appropriately enclosed within actions to adhere to MobX best practices and ensure proper reactivity.packages/editor/src/core/types/image.ts (1)
5-5: Update ofUploadImagetype signature is appropriateThe inclusion of
blockIdin theUploadImagetype allows image uploads to be associated with specific editor blocks, enhancing upload management and tracking.web/core/hooks/store/use-editor-asset.ts (1)
6-10: Implementation ofuseEditorAssethook is correctThe custom hook correctly accesses the
StoreContextand returnseditorAssetStore. The error handling ensures that it's used within aStoreProvider, preventing potential undefined behavior.packages/editor/src/core/types/config.ts (1)
3-6: LGTM! Clear separation of read-only file handling concerns.The introduction of
TReadOnlyFileHandlertype effectively separates read-only operations from editable ones.packages/editor/src/core/extensions/image/read-only-image.tsx (1)
6-8: LGTM! Proper adoption of the new type system.The change from
Pick<TFileHandler, "getAssetSrc">toTReadOnlyFileHandleraligns well with the new type separation.web/core/hooks/store/index.ts (1)
10-10: LGTM! Clean integration of the new editor asset hook.The export is properly placed and maintains the alphabetical ordering of the barrel file.
space/core/components/editor/rich-text-read-only-editor.tsx (1)
15-15: LGTM! Implementation maintains consistency.The changes mirror those in the lite editor, maintaining consistency across components. The addition of
workspaceIdis properly integrated withgetReadOnlyEditorFileHandlers.Also applies to: 19-26
packages/editor/src/core/types/collaboration.ts (1)
12-12: LGTM! Type separation improves clarity.The separation of
TReadOnlyFileHandlerfromTFileHandlerand its integration inTReadOnlyCollaborativeEditorPropsimproves type safety and clarity. This change aligns perfectly with the PR objective of separating types for editable and read-only file handlers.Also applies to: 47-47
space/core/components/issues/peek-overview/issue-details.tsx (1)
38-38: LGTM! Proper null handling.The nullish coalescing operator ensures safe handling of undefined workspaceID.
packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts (2)
7-9: LGTM! Improved type safety with dedicated read-only handler.The change from
Pick<TFileHandler>toTReadOnlyFileHandlerbetter represents the read-only nature of this component.
59-59: Verify upload status initialization.The empty object initialization for
assetsUploadStatusis correct, but ensure that consumers properly handle this empty state.✅ Verification successful
Empty object initialization for
assetsUploadStatusis safe and properly handledThe codebase shows proper handling of the empty state through type safety, null checks, and robust state management patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for places where assetsUploadStatus is accessed rg -A 3 "assetsUploadStatus" --type tsLength of output: 5806
web/core/components/editor/rich-text-editor/rich-text-read-only-editor.tsx (3)
8-9: LGTM! Improved code organization with hook.Moving to
useEditorConfighook enhances maintainability and reusability.Also applies to: 26-27
17-17: LGTM! Type safety enhancement.Adding workspaceId to props type ensures proper type checking.
34-36: LGTM! Consistent workspace context propagation.Properly passing workspace context to file handlers.
web/core/components/editor/lite-text-editor/lite-text-read-only-editor.tsx (1)
8-9: LGTM! Consistent implementation with rich text editor.The changes mirror those in the rich text editor, maintaining consistency across editor variants:
- Added workspaceId to props
- Switched to useEditorConfig hook
- Updated file handler configuration
Also applies to: 17-17, 26-27, 34-36
space/helpers/editor.helper.ts (2)
Line range hint
28-47: LGTM! Well-structured read-only file handler implementation.The separation of read-only file handlers with distinct responsibilities (
getAssetSrcandrestore) improves code organization and maintainability.
54-75: LGTM! Clean composition of file handlers.Good use of composition by spreading read-only handlers and adding editable-specific functionality.
packages/editor/src/core/components/editors/document/read-only-editor.tsx (1)
13-19: LGTM! Improved type safety for read-only editor.The use of
TReadOnlyFileHandlerinstead ofPick<TFileHandler, "getAssetSrc">provides better type safety and aligns with the separation of concerns.Also applies to: 29-29
space/core/components/editor/lite-text-editor.tsx (1)
3-3: LGTM! Improved type consistency.Good use of
TFileHandler["upload"]type to ensure consistency with the file handler interface.Also applies to: 17-17
web/core/hooks/editor/use-editor-config.ts (2)
21-96: LGTM! Well-structured editor configuration hook.The hook provides a clean separation between read-only and editable file handlers, with proper dependency management in useCallback hooks.
37-44:⚠️ Potential issueAdd null check for optional projectId.
The
projectIdparameter is optional but used without a null check ingetAssetSrc.Add null check to handle cases where
projectIdis undefined:return ( getEditorAssetSrc({ assetId: path, - projectId, + projectId: projectId ?? undefined, workspaceSlug, }) ?? "" );Likely invalid or redundant comment.
space/core/components/issues/peek-overview/comment/add-comment.tsx (1)
Line range hint
93-97: LGTM! The updated file upload handler correctly implements the new signature.The changes align with the refactoring objectives, maintaining proper state management and error handling.
packages/editor/src/core/extensions/read-only-extensions.tsx (2)
32-33: LGTM! Type changes maintain consistency with read-only pattern.The switch to
TReadOnlyFileHandleraligns with the type separation objectives.Also applies to: 37-37
97-102: LGTM! Simplified image extension configuration.Direct passing of
fileHandlerto both image extensions improves code clarity.web/ce/components/pages/editor/ai/ask-pi-menu.tsx (2)
9-10: LGTM! Workspace context integration looks good.The addition of the workspace context through the
useWorkspacehook and the derivedworkspaceIdenhances the component's integration with the workspace-specific functionality.Also applies to: 24-27
49-51: Verify error handling for undefined workspaceId.While the implementation looks good, consider the case where
workspaceIdis an empty string. Ensure that theRichTextReadOnlyEditorcomponent handles this gracefully.web/core/components/pages/version/editor.tsx (2)
12-13: LGTM! Clean hooks integration.The integration of
useEditorConfiganduseWorkspacehooks, along with the workspace context derivation, is well-structured and follows React best practices.Also applies to: 30-33, 36-37
106-108: Verify the impact of workspaceId on file handling.The addition of
workspaceIdto the file handler configuration looks good. However, let's verify that all file operations handle undefined workspace IDs gracefully.✅ Verification successful
Workspace ID handling in file operations is properly implemented
The codebase demonstrates robust handling of undefined workspace IDs through consistent null coalescing and type-safe implementations across both read-only and editable file handlers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for file handler implementations to verify workspace ID handling ast-grep --pattern 'getReadOnlyEditorFileHandlers({ $$$ workspaceId: $_ $$$ })'Length of output: 87
Script:
#!/bin/bash # Search for file handler related code rg -i "filehandler" -A 5 -B 5 # Also search for workspace ID usage in TypeScript files rg "workspaceId.*file" -g "*.ts" -g "*.tsx" -A 5 -B 5Length of output: 66031
web/core/components/issues/description-input.tsx (2)
18-18: LGTM! Clean hook integration.The addition of
useEditorAssethook is well-placed and follows React best practices.Also applies to: 54-55
155-157: LGTM! Proper workspace context integration.The addition of
workspaceIdtoRichTextReadOnlyEditoris consistent with the broader refactoring pattern.packages/editor/src/core/extensions/custom-image/custom-image.ts (1)
24-25: LGTM: Command interface updates align with the PR objectives.The addition of
blockIdparameter touploadImageand the newupdateAssetsUploadStatuscommand enhance the file handling capabilities as intended.packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx (1)
71-71: LGTM: Proper integration of blockId parameter.The addition of
blockIdparameter touseUploaderhook is consistent with the file handling refactor.web/core/store/root.store.ts (1)
10-10: LGTM: Clean integration of EditorAssetStore.The EditorAssetStore is properly integrated into the root store with appropriate initialization in both constructor and resetOnSignOut method.
Also applies to: 64-64, 94-94, 127-127
web/core/store/issue/issue-details/attachment.store.ts (1)
129-129: LGTM: Improved encapsulation of debouncedUpdateProgress.Making the method private is a good practice as it's an internal implementation detail.
web/core/components/issues/issue-detail/issue-activity/comments/comment-card.tsx (2)
162-164: LGTM! File upload handling improved with block-specific tracking.The updated
uploadFilefunction now correctly associates uploads with specific blocks, improving tracking and management of file uploads within the editor.
204-207: LGTM! Enhanced workspace context handling.The addition of
workspaceIdprop toLiteTextReadOnlyEditorimproves context handling and maintains consistency with other components.web/core/services/file.service.ts (2)
68-69: LGTM! Enhanced file upload with progress tracking.The addition of
uploadProgressHandlerenables real-time upload progress monitoring, improving user experience during file uploads.Also applies to: 79-83
131-132: LGTM! Consistent progress tracking implementation.The same progress tracking capability is correctly implemented for project assets, maintaining consistency across the service.
Also applies to: 142-146
web/core/components/profile/activity/activity-list.tsx (2)
18-18: LGTM! Improved workspace context handling.The addition of workspace context retrieval with proper null checks enhances type safety and maintains consistency.
Also applies to: 30-32
85-87: LGTM! Consistent workspace context propagation.The
workspaceIdis correctly passed to the editor component, maintaining consistency with other components.web/core/components/core/modals/gpt-assistant-popover.tsx (3)
9-10: Improved type safety by using specific editor ref type.Replacing
anywithEditorReadOnlyRefApienhances type safety and maintainability.Also applies to: 57-58
28-31: LGTM! Enhanced workspace context handling in props.The addition of workspace-related props improves component integration and maintains consistency.
Also applies to: 48-51
225-228: LGTM! Consistent workspace context propagation to editors.The
workspaceIdis correctly passed to both editor instances, maintaining consistency across the component.Also applies to: 238-241
web/core/components/profile/activity/profile-activity-list.tsx (1)
106-107: LGTM! Workspace identifiers are properly passed.The changes correctly pass both
workspaceIdandworkspaceSlugto theRichTextReadOnlyEditorcomponent, maintaining consistency with the refactored file handling system.packages/editor/src/core/hooks/use-editor.ts (1)
128-134: LGTM! Upload status sync is implemented.The new effect properly syncs the assets upload status between the file handler and editor commands.
web/ce/components/pages/editor/ai/menu.tsx (1)
24-24: LGTM! Workspace context is properly integrated.The changes correctly:
- Add
workspaceIdto the Props type- Destructure it from props
- Pass it to the
RichTextReadOnlyEditorcomponentAlso applies to: 62-62, 219-220
packages/editor/src/core/extensions/custom-image/components/image-block.tsx (1)
7-7: LGTM! Upload status visualization is implemented.The changes properly integrate the ImageUploadStatus component to show upload progress for images.
Also applies to: 214-215, 276-276
packages/editor/src/core/hooks/use-file-upload.ts (2)
17-17: LGTM!The destructuring properly includes the new
blockIdparameter.
9-9: Verify all consumers of useUploader hook provide blockId.The addition of the required
blockIdproperty is a breaking change that affects all consumers of theuseUploaderhook.Let's verify all consumers are updated:
✅ Verification successful
✓ All consumers of useUploader hook are properly updated
The only consumer in
packages/editor/src/core/extensions/custom-image/components/image-uploader.tsxcorrectly provides theblockIdparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all usages of useUploader to ensure they provide blockId ast-grep --pattern 'useUploader({$$$})'Length of output: 799
| uploadFile: async (blockId, file) => { | ||
| const { asset_id } = await uploadEditorAsset({ | ||
| blockId, | ||
| data: { | ||
| entity_identifier: id ?? "", | ||
| entity_type: EFileAssetType.PAGE_DESCRIPTION, | ||
| }, | ||
| file | ||
| ); | ||
| file, | ||
| projectId: projectId?.toString() ?? "", | ||
| workspaceSlug: workspaceSlug?.toString() ?? "", | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to the uploadFile function.
The uploadFile function should include error handling to gracefully handle upload failures and provide user feedback.
uploadFile: async (blockId, file) => {
+ try {
const { asset_id } = await uploadEditorAsset({
blockId,
data: {
entity_identifier: id ?? "",
entity_type: EFileAssetType.PAGE_DESCRIPTION,
},
file,
projectId: projectId?.toString() ?? "",
workspaceSlug: workspaceSlug?.toString() ?? "",
});
return asset_id;
+ } catch (error) {
+ console.error("Error uploading file:", error);
+ throw new Error("Failed to upload file. Please try again.");
+ }
},📝 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.
| uploadFile: async (blockId, file) => { | |
| const { asset_id } = await uploadEditorAsset({ | |
| blockId, | |
| data: { | |
| entity_identifier: id ?? "", | |
| entity_type: EFileAssetType.PAGE_DESCRIPTION, | |
| }, | |
| file | |
| ); | |
| file, | |
| projectId: projectId?.toString() ?? "", | |
| workspaceSlug: workspaceSlug?.toString() ?? "", | |
| }); | |
| uploadFile: async (blockId, file) => { | |
| try { | |
| const { asset_id } = await uploadEditorAsset({ | |
| blockId, | |
| data: { | |
| entity_identifier: id ?? "", | |
| entity_type: EFileAssetType.PAGE_DESCRIPTION, | |
| }, | |
| file, | |
| projectId: projectId?.toString() ?? "", | |
| workspaceSlug: workspaceSlug?.toString() ?? "", | |
| }); | |
| return asset_id; | |
| } catch (error) { | |
| console.error("Error uploading file:", error); | |
| throw new Error("Failed to upload file. Please try again."); | |
| } | |
| }, |
| // derived values | ||
| const { id: pageId, name: pageTitle, isContentEditable, updateTitle } = page; | ||
| const workspaceId = getWorkspaceBySlug(workspaceSlug)?.id ?? ""; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for workspaceId derivation.
The workspaceId derivation should include a null check to prevent potential runtime errors.
- const workspaceId = getWorkspaceBySlug(workspaceSlug)?.id ?? "";
+ const workspace = getWorkspaceBySlug(workspaceSlug);
+ if (!workspace?.id) {
+ console.error("Failed to get workspace ID");
+ }
+ const workspaceId = workspace?.id ?? "";📝 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 workspaceId = getWorkspaceBySlug(workspaceSlug)?.id ?? ""; | |
| const workspace = getWorkspaceBySlug(workspaceSlug); | |
| if (!workspace?.id) { | |
| console.error("Failed to get workspace ID"); | |
| } | |
| const workspaceId = workspace?.id ?? ""; |
| @@ -76,8 +72,10 @@ export const IssueDescriptionEditor: React.FC<TIssueDescriptionEditorProps> = ob | |||
| const [iAmFeelingLucky, setIAmFeelingLucky] = useState(false); | |||
| // store hooks | |||
| const { getWorkspaceBySlug } = useWorkspace(); | |||
| const workspaceId = getWorkspaceBySlug(workspaceSlug?.toString())?.id as string; | |||
| const workspaceId = getWorkspaceBySlug(workspaceSlug?.toString())?.id ?? ""; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate workspace ID before usage.
Using an empty string as a fallback for workspaceId could lead to silent failures. Consider throwing an error or handling the missing workspace ID case explicitly.
- const workspaceId = getWorkspaceBySlug(workspaceSlug?.toString())?.id ?? "";
+ const workspace = getWorkspaceBySlug(workspaceSlug?.toString());
+ const workspaceId = workspace?.id;
+ if (!workspaceId) {
+ console.error("Workspace not found for slug:", workspaceSlug);
+ }📝 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 workspaceId = getWorkspaceBySlug(workspaceSlug?.toString())?.id ?? ""; | |
| const workspace = getWorkspaceBySlug(workspaceSlug?.toString()); | |
| const workspaceId = workspace?.id; | |
| if (!workspaceId) { | |
| console.error("Workspace not found for slug:", workspaceSlug); | |
| } |
| delete this.assetsUploadStatus[blockId]; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Ensure proper mutation of observables when deleting properties
Directly deleting properties from an observable object may not trigger reactivity in MobX. To ensure that observers are notified when an upload status is removed, consider using runInAction with the set function to assign undefined, or use an observable Map for assetsUploadStatus.
Apply this diff to modify the deletion:
runInAction(() => {
- delete this.assetsUploadStatus[blockId];
+ set(this.assetsUploadStatus, blockId, undefined);
});Alternatively, initialize assetsUploadStatus as an observable map:
- assetsUploadStatus: Record<string, TAttachmentUploadStatus> = {};
+ assetsUploadStatus = observable.map<string, TAttachmentUploadStatus>();And update usages accordingly.
Committable suggestion skipped: line range outside the PR's diff.
| anchor, | ||
| workspaceId, | ||
| }), | ||
| getAssetUploadStatus: () => 0, |
There was a problem hiding this comment.
Fix hardcoded upload status.
The getAssetUploadStatus method returns a hardcoded value of 0, which doesn't reflect the actual upload status.
Consider implementing proper upload status tracking or using the upload status from the asset store:
- getAssetUploadStatus: () => 0,
+ getAssetUploadStatus: (blockId: string) => assetStore.getUploadStatus(blockId),Committable suggestion skipped: line range outside the PR's diff.
| await fileService.deleteNewAsset( | ||
| getEditorAssetSrc({ | ||
| assetId: src, | ||
| projectId, | ||
| workspaceSlug, | ||
| }) ?? "" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add null check for optional projectId in delete method.
Similar to getAssetSrc, the delete method uses projectId without a null check.
Add null check:
await fileService.deleteNewAsset(
getEditorAssetSrc({
assetId: src,
- projectId,
+ projectId: projectId ?? undefined,
workspaceSlug,
}) ?? ""
);📝 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.
| await fileService.deleteNewAsset( | |
| getEditorAssetSrc({ | |
| assetId: src, | |
| projectId, | |
| workspaceSlug, | |
| }) ?? "" | |
| ); | |
| } | |
| await fileService.deleteNewAsset( | |
| getEditorAssetSrc({ | |
| assetId: src, | |
| projectId: projectId ?? undefined, | |
| workspaceSlug, | |
| }) ?? "" | |
| ); | |
| } |
| uploadImage: (blockId, file) => async () => { | ||
| const fileUrl = await upload(blockId, file); | ||
| return fileUrl; | ||
| }, | ||
| getImageSource: (path: string) => async () => await getAssetSrc(path), | ||
| restoreImage: (src: string) => async () => { | ||
| updateAssetsUploadStatus: (updatedStatus) => () => { | ||
| this.storage.assetsUploadStatus = updatedStatus; | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Add error handling to uploadImage command
The uploadImage command should include try-catch block to handle upload failures gracefully, consistent with other error handling patterns in the codebase:
uploadImage: (blockId, file) => async () => {
try {
const fileUrl = await upload(blockId, file);
return fileUrl;
} catch (error) {
console.error("Error uploading image:", error);
throw error;
}
},🔗 Analysis chain
Verify error handling in uploadImage command.
The uploadImage command should handle potential upload failures gracefully.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in upload-related code
rg -A 5 "upload.*catch|upload.*error"
Length of output: 4859
Script:
#!/bin/bash
# Find upload function implementation and its usage
ast-grep --pattern 'function upload($$$) { $$$ }'
ast-grep --pattern 'const upload = ($$$) => { $$$ }'
rg -A 10 "uploadImage.*try|try.*uploadImage"
Length of output: 153
Script:
#!/bin/bash
# Find the complete implementation and imports
fd "custom-image.ts" --exec cat {}
rg -g "custom-image*" "try|catch|error"
Length of output: 7867
| @@ -49,7 +50,7 @@ export const useUploader = (args: TUploaderArgs) => { | |||
| reader.readAsDataURL(fileWithTrimmedName); | |||
| // @ts-expect-error - TODO: fix typings, and don't remove await from | |||
| // here for now | |||
| const url: string = await editor?.commands.uploadImage(fileWithTrimmedName); | |||
| const url: string = await editor?.commands.uploadImage(blockId, fileWithTrimmedName); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the TODO comment and improve error handling.
Several improvements needed:
- The TODO comment about typings should be addressed.
- The error handling could be more specific about what went wrong during upload.
Consider this improvement:
- // @ts-expect-error - TODO: fix typings, and don't remove await from
- // here for now
- const url: string = await editor?.commands.uploadImage(blockId, fileWithTrimmedName);
+ if (!editor?.commands.uploadImage) {
+ throw new Error("Upload command not available");
+ }
+ const url = await editor.commands.uploadImage(blockId, fileWithTrimmedName);📝 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 url: string = await editor?.commands.uploadImage(blockId, fileWithTrimmedName); | |
| if (!editor?.commands.uploadImage) { | |
| throw new Error("Upload command not available"); | |
| } | |
| const url = await editor.commands.uploadImage(blockId, fileWithTrimmedName); |
Description
This PR refactors the way editor handles files-
restorefunction to read-only file handler.asset.storein the web app to store asset upload status and handle editor uploads for scalability.New features added-
Type of Change
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the release notes:
New Features
Improvements
Technical Updates
These changes primarily focus on enhancing the editor's file upload and asset management capabilities, providing more granular control and improved user experience.