Fix image upload on articles #1126
Conversation
Adds safe actions for actions Uses actions instead of trpc
|
@NiallJoeMaher is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve significant modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CreateComponent
participant UploadService
participant AuthService
User->>CreateComponent: Initiate Upload
CreateComponent->>UploadService: Request Upload URL
UploadService->>AuthService: Validate User Session
AuthService-->>UploadService: Return Session Info
UploadService-->>CreateComponent: Provide Presigned URL
CreateComponent->>User: Display Upload URL
Possibly related PRs
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 (
|
NiallJoeMaher
temporarily deployed
to
production
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
server/lib/safeAction.ts (1)
7-15: Excellent implementation of authenticated action client.The
authActionClientsetup effectively ensures that only authenticated users can execute actions, which is crucial for secure image uploads. The error handling and user context passing are well-implemented.Consider enhancing the error message to provide more context:
- throw new Error("Session invalid."); + throw new Error("Authentication required. Please log in to perform this action.");This more descriptive error message could help in debugging and provide clearer feedback if exposed to the user interface.
server/api/router/profile.ts (4)
Line range hint
25-63: Consider renaming theeditprocedure for clarity.The
editprocedure handles various aspects of profile updates, including newsletter subscription management. While the implementation looks correct, a more descriptive name likeupdateProfileSettingsmight better reflect its functionality.
Line range hint
64-81: LGTM: Profile photo URL update logic.The
updateProfilePhotoUrlprocedure correctly updates the user's profile image URL and includes a unique identifier to prevent caching issues. Consider adding a comment explaining the purpose of appending the unique identifier to the URL for better code documentation.
Line range hint
82-117: Consider moving accepted formats to a constant.The
getUploadUrlprocedure correctly handles file type and size validation before generating a presigned URL. To improve maintainability, consider moving theacceptedFormatsarray to a constant, possibly in a separate configuration file.
Line range hint
133-201: Consider improving error message for existing email.The
updateEmailprocedure includes comprehensive checks and validations for email change requests. However, the error message for an existing email ("Unable to process the request") could be more descriptive without revealing sensitive information.Consider updating the error message to something like:
message: "This email address cannot be used. Please try a different one.",This provides more context to the user without disclosing that the email is already in use.
app/(app)/create/[[...paramsArr]]/_client.tsx (1)
97-98: Consider resetting the file input after successful uploadAfter a successful upload, you might want to reset the file input to clear the selected file, improving the user experience.
Apply this diff to reset the file input:
setUploadStatus("success"); setUploadUrl(fileLocation); + e.target.value = "";app/actions/getUploadUrl.ts (2)
28-30: Use a constant for the maximum file size for clarityInstead of hardcoding
1048576 * 10, define a constant for the maximum file size to enhance readability and maintainability.Apply this diff:
+ const MAX_FILE_SIZE = 1048576 * 10; // 10 MB if (size > 1048576 * 10) { throw new Error("Maximum file size 10mb"); }And update the condition:
- if (size > 1048576 * 10) { + if (size > MAX_FILE_SIZE) { throw new Error("Maximum file size 10mb"); }
39-43: Avoid logging errors to the console in productionLogging errors to the console can expose sensitive information in a production environment. Since the error is already captured by Sentry, consider removing the
console.errorstatement.Apply this diff:
Sentry.captureException(error); - console.error("Error getting presigned URL:", error); throw new Error("Failed to upload image.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonpackage.jsonis excluded by!**/*.json
📒 Files selected for processing (4)
- app/(app)/create/[[...paramsArr]]/_client.tsx (2 hunks)
- app/actions/getUploadUrl.ts (1 hunks)
- server/api/router/profile.ts (1 hunks)
- server/lib/safeAction.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (10)
server/lib/safeAction.ts (2)
1-5: LGTM! Well-structured setup for safe actions.The imports and basic setup are well-organized and align perfectly with the PR objective of introducing safe actions for handling image uploads. The use of the "server-only" directive ensures proper security by restricting this code to server-side execution.
1-15: Excellent implementation of safe actions for image uploads.This new file successfully introduces safe actions for handling image uploads, directly addressing the PR objectives. The implementation is concise, well-structured, and follows best practices for server-side security.
Key strengths:
- Server-only execution ensures sensitive operations are protected.
- Integration with existing authentication system.
- Proper error handling for unauthenticated requests.
This implementation provides a solid foundation for secure image upload handling in the Codu platform.
server/api/router/profile.ts (3)
Line range hint
118-132: LGTM: User profile retrieval logic.The
getprocedure correctly retrieves a user profile based on the provided username and includes appropriate error handling for non-existent profiles.
Line range hint
1-201: Overall, the profileRouter implementation is robust and well-structured.The changes made to this file (removal of unused import) are appropriate and do not negatively impact the functionality. The procedures within the profileRouter are well-implemented with proper input validation, error handling, and security considerations such as rate limiting.
Minor suggestions have been made for improving naming conventions, error messages, and code organization, but these are not critical issues. The current implementation should work as intended for handling various profile-related operations.
1-1: LGTM: Removal of unused import.The removal of the unused
emailChangeHistoryimport improves code cleanliness. This change is appropriate as the import is not used within this file.Let's verify that
emailChangeHistoryis not used elsewhere in the codebase:✅ Verification successful
LGTM: Removal of unused import.
The removal of the unused
emailChangeHistoryimport inserver/api/router/profile.tsimproves code cleanliness and has no impact on other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usages of emailChangeHistory in the codebase # Test: Search for emailChangeHistory usage rg --type typescript 'emailChangeHistory'Length of output: 76
Script:
#!/bin/bash # Description: Check for any remaining usages of emailChangeHistory in TypeScript files # Test: Search for emailChangeHistory usage in .ts and .tsx files rg 'emailChangeHistory' --glob '*.ts' --glob '*.tsx'Length of output: 516
app/(app)/create/[[...paramsArr]]/_client.tsx (4)
32-32: Import statement is appropriateThe import of
getUploadUrlfrom"@/app/actions/getUploadUrl"is correctly added and aligns with the usage in the code.
34-34: Removal ofsessionprop is appropriateThe
Createcomponent no longer requires thesessionprop, simplifying its signature. This change is acceptable if thesessionis not used within the component.
74-82:⚠️ Potential issueVerify the response structure of
getUploadUrlIn line 75,
getUploadUrlis called, and in line 81, you're accessingres?.datato get thesignedUrl. Please verify thatgetUploadUrlreturns an object with adataproperty containing thesignedUrl. IfgetUploadUrlreturns the signed URL directly, you should assign it directly without accessingdata.Run the following script to verify the return value of
getUploadUrl:
90-96:⚠️ Potential issueEnsure correct handling of
uploadFileresponseIn line 90, you're destructuring
{ fileLocation }from the response ofuploadFile(signedUrl, file). Please verify thatuploadFilereturns an object containingfileLocation. If it returns the file location directly or under a different property name, you may need to adjust the destructuring accordingly.Run the following script to verify the return value of
uploadFilefunction:app/actions/getUploadUrl.ts (1)
15-44: Verify all usages ofgetUploadUrlhandle the new validation correctlyEnsure that all calls to
getUploadUrlproperly handle the new validations fortypeandsize, and that appropriate error handling is in place where this action is used.Run the following script to find all usages of
getUploadUrl:
| setUploadStatus("error"); | ||
| toast.error( | ||
| error instanceof Error | ||
| ? error.message | ||
| : "An error occurred while uploading the image.", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include error logging for better debugging
In your catch block, consider logging the error using Sentry or console logging. This can aid in debugging issues that occur during the upload process.
Apply this diff to log the error:
} catch (error) {
setUploadStatus("error");
toast.error(
error instanceof Error
? error.message
: "An error occurred while uploading the image.",
);
+ Sentry.captureException(error);
}📝 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.
| setUploadStatus("error"); | |
| toast.error( | |
| error instanceof Error | |
| ? error.message | |
| : "An error occurred while uploading the image.", | |
| ); | |
| } | |
| setUploadStatus("error"); | |
| toast.error( | |
| error instanceof Error | |
| ? error.message | |
| : "An error occurred while uploading the image.", | |
| ); | |
| Sentry.captureException(error); | |
| } |
| const { type, size, uploadType } = parsedInput; | ||
| const extension = type.split("/")[1]; | ||
| const acceptedFormats = ["jpg", "jpeg", "gif", "png", "webp"]; |
There was a problem hiding this comment.
Ensure type contains a slash to prevent potential errors
In line 19, extension is derived using type.split("/")[1]. If type does not contain a " / ", extension will be undefined, which may cause issues when checking acceptedFormats. Consider validating that type includes a slash before splitting or handling the case where extension is undefined.
Apply this diff to add validation:
+ if (!type.includes('/')) {
+ throw new Error("Invalid file type format.");
+ }
const extension = type.split("/")[1];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { type, size, uploadType } = parsedInput; | |
| const extension = type.split("/")[1]; | |
| const acceptedFormats = ["jpg", "jpeg", "gif", "png", "webp"]; | |
| const { type, size, uploadType } = parsedInput; | |
| if (!type.includes('/')) { | |
| throw new Error("Invalid file type format."); | |
| } | |
| const extension = type.split("/")[1]; | |
| const acceptedFormats = ["jpg", "jpeg", "gif", "png", "webp"]; |
NiallJoeMaher
temporarily deployed
to
production
GitHub Actions
Inactive
✨ Codu Pull Request 💻
Pull Request details
Fix image upload on articles