Feate/s3 cloud dev #5
Conversation
WalkthroughThe changes involve the introduction of new configurations, templates, and components across the project. YAML configurations, issue and pull request templates, and settings for various tools like ESLint and Prettier have been added or modified. There's a focus on UI components with the addition of icons and share buttons for social media, particularly email and Facebook. Backend changes include new routes, services, and error handling for uploads, as well as S3 integration for file management. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 19
Configuration used: .coderabbit.yaml
Files ignored due to filter (5)
- apps/admin/package.json
- package.json
- packages/krs-share-icons/package.json
- packages/krs-share-icons/tsconfig.json
- pnpm-lock.yaml
Files selected for processing (45)
- .coderabbit.yaml (1 hunks)
- .editorconfig (1 hunks)
- .github/ISSUE_TEMPLATE/bug.yml (1 hunks)
- .github/ISSUE_TEMPLATE/feature-request.yml (1 hunks)
- .github/ISSUE_TEMPLATE/refactor.yml (1 hunks)
- .github/PULL_REQUEST_TEMPLATE/bug.md (1 hunks)
- .github/PULL_REQUEST_TEMPLATE/feature.md (1 hunks)
- .vscode/settings.json (1 hunks)
- apps/admin/postcss.config.js (1 hunks)
- apps/admin/src/components/core/icons.tsx (3 hunks)
- apps/admin/src/components/layouts/styles/index.ts (1 hunks)
- apps/admin/src/components/providers/index.tsx (1 hunks)
- apps/admin/src/index.css (1 hunks)
- apps/admin/src/main.tsx (1 hunks)
- apps/admin/src/views/settings/system/index.tsx (1 hunks)
- apps/admin/tailwind.config.js (1 hunks)
- apps/backend/src/app.ts (1 hunks)
- apps/backend/src/features/uploads/controllers/folder.controller.ts (1 hunks)
- apps/backend/src/features/uploads/enums/enums.upload.ts (1 hunks)
- apps/backend/src/features/uploads/enums/error.upload.ts (1 hunks)
- apps/backend/src/features/uploads/interfaces/folder.interface.ts (1 hunks)
- apps/backend/src/features/uploads/models/folder.schema.ts (2 hunks)
- apps/backend/src/features/uploads/models/image.schema.ts (1 hunks)
- apps/backend/src/features/uploads/routes/upload.route.ts (1 hunks)
- apps/backend/src/features/uploads/services/folder.service.ts (1 hunks)
- apps/backend/src/setupRoutes.ts (1 hunks)
- apps/backend/src/shared/globals/helpers/s3-upload.ts (1 hunks)
- packages/krs-share-icons/.eslintrc (1 hunks)
- packages/krs-share-icons/src/component/email/email-icon.tsx (1 hunks)
- packages/krs-share-icons/src/component/email/email-share-button.tsx (1 hunks)
- packages/krs-share-icons/src/component/email/index.ts (1 hunks)
- packages/krs-share-icons/src/component/facebook/facebook-icon.tsx (1 hunks)
- packages/krs-share-icons/src/component/facebook/facebook-messenger-icon.tsx (1 hunks)
- packages/krs-share-icons/src/component/facebook/facebook-messenger-share-button.tsx (1 hunks)
- packages/krs-share-icons/src/component/facebook/facebook-share-button.tsx (1 hunks)
- packages/krs-share-icons/src/component/facebook/index.ts (1 hunks)
- packages/krs-share-icons/src/component/index.ts (1 hunks)
- packages/krs-share-icons/src/component/share-button.tsx (1 hunks)
- packages/krs-share-icons/src/hocs/create-icon.tsx (1 hunks)
- packages/krs-share-icons/src/hocs/create-share-button.tsx (1 hunks)
- packages/krs-share-icons/src/hocs/create-share-count.tsx (1 hunks)
- packages/krs-share-icons/src/hooks/use-is-mounted.ts (1 hunks)
- packages/krs-share-icons/src/index.ts (1 hunks)
- packages/krs-share-icons/src/types/index.ts (1 hunks)
- packages/krs-share-icons/src/utils/index.ts (1 hunks)
Files skipped from review due to trivial changes (7)
- .github/ISSUE_TEMPLATE/refactor.yml
- .github/PULL_REQUEST_TEMPLATE/bug.md
- .github/PULL_REQUEST_TEMPLATE/feature.md
- apps/admin/src/main.tsx
- apps/backend/src/features/uploads/models/image.schema.ts
- packages/krs-share-icons/.eslintrc
- packages/krs-share-icons/src/component/email/email-icon.tsx
Additional comments: 54
.coderabbit.yaml (1)
- 1-23: The configuration in
.coderabbit.yamlappears to be well-formed and there are no syntax errors. Ensure that the settings align with the project's review and CI requirements..editorconfig (2)
17-19: The summary did not mention the addition of settings for Markdown files. This change disables the maximum line length and stops trimming trailing whitespace for Markdown files, which could be important for contributors to know.
21-22: The addition of indentation settings for YAML files is consistent with the summary and is a good practice for ensuring consistent formatting in YAML files.
.github/ISSUE_TEMPLATE/bug.yml (1)
- 1-17: The addition of the bug report issue template is correctly implemented and will help standardize the bug reporting process.
.github/ISSUE_TEMPLATE/feature-request.yml (1)
- 1-23: The new GitHub issue template for feature requests is well-structured and includes necessary fields such as a description of the feature, its purpose, and resources needed. The assignees field is pre-populated with a specific user, which may need to be adjusted based on team practices.
.vscode/settings.json (1)
- 1-22: The changes in
.vscode/settings.jsoncorrectly reflect the switch topnpmand adjustments to ESLint and Prettier configurations, as well as file handling settings. These changes are consistent with the PR objectives and the summary provided.apps/admin/postcss.config.js (1)
- 1-6: The configuration for Tailwind CSS and Autoprefixer has been correctly set up in the
postcss.config.jsfile.apps/admin/src/components/core/icons.tsx (2)
6-10: The addition of
Minusto the list of imports from 'lucide-react' and its inclusion in theIconsobject is correctly implemented.129-129: The
Iconsobject has been modified to renameSettingstoSetting. Verify if this change is intentional and consistent throughout the codebase.apps/admin/src/components/layouts/styles/index.ts (1)
- 11-13: The changes to
StyledContainerare consistent with the summary provided and there are no apparent issues with the code itself.apps/admin/src/components/providers/index.tsx (1)
- 64-75: The addition of the
Typographyconfiguration within theKRSProviderscomponent is correctly implemented and follows the stated objectives. The configuration is consistent with the summary provided.apps/admin/src/index.css (1)
- 1-2: The addition of Tailwind CSS directives is correct and aligns with the project's move to incorporate Tailwind for styling.
apps/admin/src/views/settings/system/index.tsx (1)
- 1-54: The changes to the
SystemPagecomponent align with the PR objectives and the summary provided. The use ofantdcomponents and the token for styling are correctly implemented, and the component is properly exported.apps/admin/tailwind.config.js (3)
7-7: The summary indicates that Tailwind CSS and Autoprefixer plugins have been added, but the
pluginsarray in thetailwind.config.jsfile is empty. Please verify if the plugins are configured elsewhere or if they should be included here.3-3: The
contentconfiguration looks correct and should ensure that Tailwind CSS purges unused styles from the specified files.4-5: The
themeandextendobjects are empty, which is a valid configuration. Customizations to Tailwind's default theme can be added here as needed.apps/backend/src/features/uploads/controllers/folder.controller.ts (3)
10-26: The
createmethod correctly handles folder creation and upload to a cloud service, with proper error handling usingKRSResponse. The use ofHTTP_STATUS.CREATEDfor successful creation is appropriate.32-58: The
updatemethod includes logic for updating folder details and moving the folder in cloud storage. The error handling usingKRSResponseandHTTP_STATUS.BAD_REQUESTfor errors is consistent with best practices.60-66: The
deletemethod correctly usesfolderServiceto delete a folder and responds withHTTP_STATUS.OKupon successful deletion.apps/backend/src/features/uploads/enums/enums.upload.ts (1)
- 1-6: The
EUploadTypeenum has been correctly defined to include various upload types, including S3, which aligns with the PR's objective of integrating S3 cloud services.apps/backend/src/features/uploads/interfaces/folder.interface.ts (1)
- 7-7: Ensure that all parts of the application that interact with the
parent_idproperty of theIFolderDocumentinterface are updated to handle thestringtype in addition toObjectIdandnull.apps/backend/src/features/uploads/models/folder.schema.ts (3)
1-8: The imports for
Request,Response,HTTP_STATUS,IBaseError, andEErrorUploadhave been correctly added to support the new error handling logic in thefolderSchema.pre('save')hook.29-65: The
folderSchema.pre('save')hook now includes comprehensive error handling for duplicate folder names and missing parent folders, throwing errors with appropriate error codes and status codes.27-68: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [68-70]
The
FolderModelis correctly defined and exported, making it available for use elsewhere in the application.apps/backend/src/features/uploads/routes/upload.route.ts (4)
9-49: The
UploadRoutesclass is well-structured and follows good practices in terms of defining routes and middleware. The use of a validation middleware to check for valid MongoDB Object IDs before proceeding with folder-related operations is a good security practice to prevent potential injection attacks or errors.16-30: The
validateObjectIdmiddleware is correctly implemented to ensure that theidparameter in the URL is a valid MongoDB Object ID before proceeding with the request. This is a crucial check that helps prevent potential injection attacks or errors due to invalid IDs.32-45: The
routesmethod is correctly set up with appropriate HTTP methods and route paths for the/folderendpoint. The POST, PUT, and DELETE operations are mapped to their respective controller methods with the necessary middleware in place.32-45: Ensure that there is proper error handling within the
Foldercontroller methods or by a global error handler, as the routes do not have explicit error handling.apps/backend/src/features/uploads/services/folder.service.ts (5)
48-54: The method
checkS3Existis clear and concise, correctly using the helper froms3Uploadto check for the existence of a folder in S3.57-81: The
createAndUploadmethod correctly checks for the existence of the folder in S3 before attempting to create it, which is good practice to avoid overwriting existing data.84-112: The
checkFolderExistAndGetSlugmethod combines checking the existence of a folder in the database and S3, throwing appropriate errors if not found. This is a good use of error handling to prevent invalid operations.115-120: The
deleteFoldermethod properly deletes the folder from S3 before removing it from the database, which is a good practice to ensure data consistency.164-164: The
folderServiceis exported as a singleton, which is a common practice in Node.js applications for service classes.apps/backend/src/setupRoutes.ts (1)
- 10-10: The middleware for
uploadRoutesis correctly added to handle routes under theBASE_PATH. Ensure that theuploadRoutes.routes()method is properly defined and tested.packages/krs-share-icons/src/component/email/index.ts (1)
- 1-2: The export statements for
EmailIconandEmailShareButtonare correctly structured.packages/krs-share-icons/src/component/facebook/facebook-icon.tsx (1)
- 1-9: The
FacebookIconcomponent is correctly defined using thecreateIconhigher-order component with appropriate properties such ascolor,networkName, andpath.packages/krs-share-icons/src/component/facebook/facebook-messenger-icon.tsx (1)
- 1-9: The
FacebookMessengerIconcomponent has been successfully defined using thecreateIconhigher-order component with the specified color and path for the Facebook Messenger icon.packages/krs-share-icons/src/component/facebook/facebook-messenger-share-button.tsx (1)
- 1-44: The implementation of the
FacebookMessengerShareButtoncomponent appears to be correct and follows best practices for creating share buttons using higher-order components. ThefacebookMessengerLinkfunction properly constructs the URL for the Facebook Messenger dialog using the provided options, and the component is exported as expected.packages/krs-share-icons/src/component/facebook/facebook-share-button.tsx (1)
- 1-23: The implementation of the
FacebookShareButtoncomponent using thecreateShareButtonhigher-order component and utility functions likeassertandobjectToGetParamsis correct and follows good practices for modularity and error handling. The use of an options object to specify the dimensions of the popup window is also a good approach to encapsulate configuration settings.packages/krs-share-icons/src/component/facebook/index.ts (1)
- 1-4: The export statements for the Facebook-related icon and share button components are correctly implemented.
packages/krs-share-icons/src/component/index.ts (1)
- 1-3: The summary indicates that new icon and share button components have been added to the
krs-share-iconspackage, but they are not visible in the exports within the provided hunk. Please ensure that all new components are properly exported if they are intended to be part of the public API.packages/krs-share-icons/src/component/share-button.tsx (2)
74-106: The
handleClickfunction is well-structured and includes checks for thedisabledstate, asynchronous operations before click, and conditional opening of the share dialog. This is a good example of handling multiple concerns within a single event handler while keeping the code readable and maintainable.108-131: The merging of class names and styles using
cxand the spread operator is effectively done, allowing for both default and custom styles to be applied based on the component's props. This is a good practice for creating flexible and reusable components.packages/krs-share-icons/src/hocs/create-icon.tsx (1)
- 1-52: The
createIconhigher-order component is well-implemented, providing a flexible and extensible way to create SVG icons with customizable properties such as size, background style, border radius, and icon fill color. The use of TypeScript for prop types adds an additional layer of type safety.packages/krs-share-icons/src/hocs/create-share-count.tsx (1)
- 1-53: The implementation of the
createShareCounthigher-order component and theSocialMediaShareCountcomponent appears to be correct. The use of theuseIsMountedhook is a good practice to avoid setting state on an unmounted component, and thegetCountfunction is used properly to fetch the share count asynchronously. TheuseEffecthook's dependency array is correctly set to update the share count when theurlchanges, and theisLoadingstate is managed appropriately. TheclassNameand...restprops are also used correctly to allow for custom styling and passing additional props. ThedisplayNameproperty is set to aid in debugging. Overall, the code is well-structured and follows best practices for React component development.packages/krs-share-icons/src/hooks/use-is-mounted.ts (1)
- 1-15: The implementation of the
useIsMountedhook appears correct and follows best practices for tracking component mount state in React.packages/krs-share-icons/src/index.ts (1)
- 1-1: The export statement correctly re-exports all exports from the
./componentmodule. Ensure that all new components and utility functions mentioned in the summary are included in the./componentmodule or are otherwise re-exported correctly to be available for import.packages/krs-share-icons/src/types/index.ts (1)
- 1-3: The changes to the type definitions look good and there are no issues with the syntax or logic. However, the summary does not mention these changes, which is an inconsistency that should be addressed.
packages/krs-share-icons/src/utils/index.ts (6)
1-4: This function correctly checks if an object is a Promise by verifying that it's an object or function and has a
.thenmethod.6-18: The function
getBoxPositionOnWindowCentercalculates the position to center a box on the current window. It useswindow.outerWidth,window.outerHeight, and other window properties which may not work as expected in all browsers when the window is not fully visible or is minimized.20-26: The function
getBoxPositionOnScreenCentercalculates the position to center a box on the screen. It assumes thatwindow.screen.heightandwindow.screen.widthare available and give the correct screen size, which may not be the case on all devices or browsers, especially with multiple monitors.28-75: The
windowOpenfunction opens a new window and sets up an interval to detect when the window is closed. It's important to ensure that the interval is cleared when the window is closed to prevent memory leaks. The function also catches exceptions when checking if the window is closed, which is good practice.78-89: The
assertfunction throws anAssertionErrorif the provided value is falsy. This is a common pattern for enforcing invariants in the code. However, it's important to ensure that this function is not used in a way that could expose sensitive information in error messages or logs.91-101: The
objectToGetParamsfunction converts an object to a query string, filtering outundefinedandnullvalues. It correctly encodes the keys and values to ensure they are safe to include in a URL. This function seems well-implemented for its intended use.
| private static handleExit(): void { | ||
| process.on('uncaughtException', (error: Error) => { | ||
| log.error(`There was an uncaught error: ${error}`) | ||
| KRSApplication.shutDownProperly(1) | ||
| // KRSApplication.shutDownProperly(1) | ||
| }) | ||
|
|
||
| process.on('unhandleRejection', (reason: Error) => { | ||
| log.error(`Unhandled rejection at promise: ${reason}`) | ||
| KRSApplication.shutDownProperly(2) | ||
| // KRSApplication.shutDownProperly(2) | ||
| }) | ||
|
|
||
| process.on('SIGTERM', () => { | ||
| log.error('Caught SIGTERM') | ||
| KRSApplication.shutDownProperly(2) | ||
| // KRSApplication.shutDownProperly(2) | ||
| }) | ||
|
|
||
| process.on('SIGINT', () => { | ||
| log.error('Caught SIGINT') | ||
| KRSApplication.shutDownProperly(2) | ||
| // KRSApplication.shutDownProperly(2) | ||
| }) | ||
|
|
||
| process.on('exit', () => { |
There was a problem hiding this comment.
The shutdown logic in handleExit has been commented out. Ensure that this is intentional and that an alternative shutdown mechanism is in place or planned to handle these signals and errors properly.
| const BASE_PATH = '/api/v1' | ||
|
|
||
| export default (app: Application): void => { | ||
| const routes = () => {} |
There was a problem hiding this comment.
The routes function is declared but contains no implementation. If this is intentional as a placeholder, consider adding a comment to clarify. If it's meant to be implemented, complete the function or remove it if it's not needed.
- const routes = () => {}
+ // TODO: Implement routes or remove if not neededCommittable 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.
| const routes = () => {} | |
| // TODO: Implement routes or remove if not needed |
| FOLDER_NOT_FOUND = 'FOLDER_NOT_FOUND', | ||
| FOLDER_NOT_EXIST = 'FOLDER_NOT_EXIST', | ||
| FOLDER_EXISTED = 'FOLDER_EXISTED', |
There was a problem hiding this comment.
The error codes FOLDER_NOT_FOUND and FOLDER_NOT_EXIST seem to convey the same meaning. Consider consolidating them to avoid redundancy.
| FOLDER_S3_NOT_FOUND = 'FOLDER_S3_NOT_FOUND', | ||
| FOLDER_S3_NOT_EXIST = 'FOLDER_S3_NOT_EXIST', | ||
| FOLDER_S3_EXISTED = 'FOLDER_S3_EXISTED', |
There was a problem hiding this comment.
Similarly, FOLDER_S3_NOT_FOUND and FOLDER_S3_NOT_EXIST appear to be redundant. It would be best to use one term consistently to prevent confusion.
| attributes: | ||
| label: What does this MR do and why? | ||
| description: What do you want to happen? What problem are you trying to solve? | ||
| placeholder: Describe the need for the feature. |
There was a problem hiding this comment.
The placeholder text in the feature request template uses "MR" (Merge Request), which is GitLab terminology. For GitHub, it should be "PR" (Pull Request) or simply "feature".
- placeholder: Describe the need for the feature.
+ placeholder: Describe the need for the feature or improvement.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.
| placeholder: Describe the need for the feature. | |
| placeholder: Describe the need for the feature or improvement. |
| bucket: string = 'krs-storage' | ||
| s3Client: S3Client = setupStorage() | ||
|
|
||
| contructor() {} |
There was a problem hiding this comment.
The constructor is misspelled as contructor, which means it won't be recognized as a constructor in JavaScript/TypeScript.
- contructor() {}
+ constructor() {}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.
| contructor() {} | |
| constructor() {} |
| return new Promise<void>((resolve, reject) => { | ||
| this.s3Client.send(uploadCommand, (err, data) => { | ||
| if (err) { | ||
| log.error(`Error during upload: ${err}`) | ||
|
|
||
| reject(err) | ||
| } else { | ||
| log.info( | ||
| `Successfully uploaded data to ${uploadObject.Bucket}/${uploadObject.Key}` | ||
| ) | ||
|
|
||
| resolve() | ||
| } | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The upload method wraps the AWS SDK's promise-based send method in an unnecessary promise. Simplify by returning the promise directly.
- return new Promise<void>((resolve, reject) => {
- this.s3Client.send(uploadCommand, (err, data) => {
- if (err) {
- log.error(`Error during upload: ${err}`)
- reject(err)
- } else {
- log.info(`Successfully uploaded data to ${uploadObject.Bucket}/${uploadObject.Key}`)
- resolve()
- }
- })
- })
+ return this.s3Client.send(uploadCommand).then(
+ () => log.info(`Successfully uploaded data to ${uploadObject.Bucket}/${uploadObject.Key}`),
+ (err) => {
+ log.error(`Error during upload: ${err}`);
+ return Promise.reject(err);
+ }
+ );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.
| return new Promise<void>((resolve, reject) => { | |
| this.s3Client.send(uploadCommand, (err, data) => { | |
| if (err) { | |
| log.error(`Error during upload: ${err}`) | |
| reject(err) | |
| } else { | |
| log.info( | |
| `Successfully uploaded data to ${uploadObject.Bucket}/${uploadObject.Key}` | |
| ) | |
| resolve() | |
| } | |
| }) | |
| }) | |
| return this.s3Client.send(uploadCommand).then( | |
| () => log.info(`Successfully uploaded data to ${uploadObject.Bucket}/${uploadObject.Key}`), | |
| (err) => { | |
| log.error(`Error during upload: ${err}`); | |
| return Promise.reject(err); | |
| } | |
| ); |
| this.copy(slug, new_slug, type) | ||
| this.delete(slug, type) |
There was a problem hiding this comment.
The move method does not wait for the copy operation to complete before deleting, which could lead to data loss.
- this.copy(slug, new_slug, type)
- this.delete(slug, type)
+ return this.copy(slug, new_slug, type).then(() => this.delete(slug, type));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.
| this.copy(slug, new_slug, type) | |
| this.delete(slug, type) | |
| return this.copy(slug, new_slug, type).then(() => this.delete(slug, type)); |
| reject(err) | ||
| } else { | ||
| log.info( | ||
| `Successfully uploaded data to ${objectDelete.Bucket}/${objectDelete.Key}` |
There was a problem hiding this comment.
The log message in the delete method is incorrect; it should indicate successful deletion, not upload.
- `Successfully uploaded data to ${objectDelete.Bucket}/${objectDelete.Key}`
+ `Successfully deleted data from ${objectDelete.Bucket}/${objectDelete.Key}`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.
| `Successfully uploaded data to ${objectDelete.Bucket}/${objectDelete.Key}` | |
| `Successfully deleted data from ${objectDelete.Bucket}/${objectDelete.Key}` |
| return new Promise<boolean>((resolve, reject) => { | ||
| this.s3Client.send(headCommand, (err, data) => { | ||
| if (err) { | ||
| resolve(false) |
There was a problem hiding this comment.
The checkExist method resolves to false on error without logging the error, which could be misleading.
- resolve(false)
+ log.error(`Error checking existence: ${err}`);
+ reject(err);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.
| resolve(false) | |
| log.error(`Error checking existence: ${err}`); | |
| reject(err); |
2 participants
No description provided.