Conversation
|
Warning Rate limit exceeded@BengeeL has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe changes introduce local file upload support for development and switch production file storage from Firebase to AWS S3, with corresponding updates to backend services and controllers. The frontend's content editor is unified for both creation and editing, removing AI summarization, and routing is updated accordingly. Additional validation and error handling improvements are included throughout. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend (ContentEditor)
participant Backend (ContentController)
participant StorageService
participant AWS S3/Local FS
User->>Frontend (ContentEditor): Submit content (create/edit) with file
Frontend (ContentEditor)->>Backend (ContentController): POST/PUT content, multipart with file
Backend (ContentController)->>StorageService: uploadFile(file, path, name, type)
alt Development
StorageService->>Local FS: Write file to local_uploads/
Local FS-->>StorageService: Return local URL
else Production
StorageService->>AWS S3: PutObjectCommand (upload file)
AWS S3-->>StorageService: Confirm upload
StorageService->>AWS S3: Generate signed URL
AWS S3-->>StorageService: Return signed URL
end
StorageService-->>Backend (ContentController): Return file URL
Backend (ContentController)-->>Frontend (ContentEditor): Respond with content info
Frontend (ContentEditor)-->>User: Redirect or show result
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
.gitignore (1)
191-191: LGTM! Good addition for local development file storage.Adding
local_uploadsto gitignore is appropriate to prevent tracking of locally uploaded files during development. This aligns well with the local file storage implementation.Consider grouping this with other directory exclusions for better organization:
# Dependency directories node_modules/ jspm_packages/ +local_uploadsbackend/src/modules/user/controllers/user.controller.ts (1)
141-143: Good validation addition, but consider enhancing the file check.Adding file validation before upload is excellent defensive programming. However, the current check could be more robust.
Consider enhancing the validation to handle edge cases:
- if (!file) { - return res.status(400).json({ error: "No file uploaded." }); - } + if (!files.profileImage || !files.profileImage[0]) { + return res.status(400).json({ error: "No file uploaded." }); + }This prevents potential errors if
files.profileImageis undefined or empty.backend/src/modules/content/controllers/content.controller.ts (1)
451-451: Verify query parameter extraction precedence.The parentheses around the type assertion ensure correct operator precedence, but consider using a more explicit approach for better readability.
Consider this more explicit approach for better clarity:
- const userId = (req.query.userId as string) || undefined; + const userId = typeof req.query.userId === 'string' ? req.query.userId : undefined;frontend/src/pages/content/ContentEditor.tsx (1)
199-201: Move authentication check to useEffect for better practice.While the current placement works, it's better to handle navigation side effects in useEffect.
- // User must be authenticated to create content - if (!auth.isAuthenticated) { - navigate("/authentication/login"); - } + // User must be authenticated to create content + useEffect(() => { + if (!auth.isAuthenticated) { + navigate("/authentication/login"); + } + }, [auth.isAuthenticated, navigate]);backend/src/modules/storage/services/storage.service.ts (1)
240-249: Consider handling different S3 URL formats.S3 supports both path-style and virtual-hosted-style URLs. The current implementation might not handle all cases correctly.
Enhanced implementation to handle both URL styles:
static extractFilePathFromUrl(url: string): string { try { const u = new URL(url); - // Remove leading slash from pathname - return u.pathname.replace(/^\/+/, ""); + + // Handle virtual-hosted-style: https://bucket.s3.region.amazonaws.com/key + if (u.hostname.includes('.s3.') && u.hostname.includes('.amazonaws.com')) { + return u.pathname.replace(/^\/+/, ""); + } + + // Handle path-style: https://s3.region.amazonaws.com/bucket/key + if (u.hostname.startsWith('s3.') && u.hostname.includes('.amazonaws.com')) { + const pathParts = u.pathname.split('/').filter(Boolean); + pathParts.shift(); // Remove bucket name + return pathParts.join('/'); + } + + // Default case + return u.pathname.replace(/^\/+/, ""); } catch { - throw new Error("Invalid S3 URL"); + throw new Error(`Invalid S3 URL: ${url}`); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.gitignore(1 hunks)backend/package.json(2 hunks)backend/src/app.config.ts(1 hunks)backend/src/app.ts(2 hunks)backend/src/modules/content/controllers/content.controller.ts(4 hunks)backend/src/modules/storage/services/storage.service.ts(1 hunks)backend/src/modules/subscription/services/stripe.service.ts(1 hunks)backend/src/modules/user/controllers/user.controller.ts(1 hunks)frontend/src/App.tsx(1 hunks)frontend/src/pages/content/ContentEditor.tsx(5 hunks)frontend/src/pages/content/ContentView.tsx(11 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
backend/src/app.ts (1)
backend/src/shared/utils/response.ts (1)
createErrorResponse(26-36)
backend/src/modules/user/controllers/user.controller.ts (1)
backend/src/modules/storage/services/storage.service.ts (1)
StorageService(49-250)
frontend/src/App.tsx (1)
frontend/src/pages/content/ContentEditor.tsx (1)
ContentEditor(20-306)
backend/src/modules/content/controllers/content.controller.ts (1)
backend/src/modules/storage/services/storage.service.ts (1)
StorageService(49-250)
frontend/src/pages/content/ContentEditor.tsx (2)
frontend/src/scripts/api.ts (1)
apiURL(8-8)backend/src/shared/config/firebase.config.ts (1)
auth(31-31)
backend/src/app.config.ts (1)
backend/src/shared/config/environment.ts (1)
env(28-64)
backend/src/modules/storage/services/storage.service.ts (1)
backend/src/shared/utils/logger.ts (1)
logger(14-36)
🔇 Additional comments (14)
backend/src/modules/subscription/services/stripe.service.ts (1)
6-6:Details
✅ Verification successful
Verify the Stripe API version update for compatibility and breaking changes.
The API version was updated from '2025-04-30.basil' to '2025-05-28.basil'. Please ensure this version exists and verify there are no breaking changes that could affect the existing Stripe service functionality.
🌐 Web query:
Is Stripe API version 2025-05-28.basil valid and what are the differences between 2025-04-30.basil and 2025-05-28.basil?💡 Result:
Yes, Stripe API version
2025-05-28.basilis valid. This version is part of Stripe's "Basil" release, which follows their new API release process introduced in 2024. Under this process, major releases occur twice a year and include breaking changes, while monthly releases add new features without breaking changes. (stripe.com)The "Basil" release began with version
2025-03-31.basil, introducing several breaking changes and new features. Subsequent versions, including2025-04-30.basiland2025-05-28.basil, have been part of this release cycle, focusing on additive changes without introducing new breaking changes.While the specific changelog for
2025-05-28.basilisn't detailed in the provided sources, it's important to note that this version would include only additive changes compared to2025-04-30.basil, as per Stripe's versioning policy. For a comprehensive list of changes and to ensure your integration remains up-to-date, you can refer to Stripe's changelog. (docs.stripe.com)To stay informed about the latest updates and to plan your integration accordingly, regularly reviewing Stripe's changelog is recommended.
Citations:
- 1: https://stripe.com/blog/introducing-stripes-new-api-release-process?utm_source=openai
- 2: https://docs.stripe.com/changelog?utm_source=openai
Stripe API version update verified – no breaking changes
The new API version
2025-05-28.basilis valid and, per Stripe’s “Basil” release policy, only includes additive (non-breaking) changes compared to2025-04-30.basil. Your existing integration should continue to work without modification.• No breaking changes introduced in this monthly release
• Version exists and is part of Stripe’s documented release cycle
• For full details, you may review Stripe’s changelog: https://docs.stripe.com/changelog?utm_source=openaibackend/package.json (2)
35-36: LGTM! AWS SDK packages added for S3 integration.The addition of
@aws-sdk/client-s3and@aws-sdk/s3-request-presignerpackages at version 3.816.0 is appropriate for implementing AWS S3 storage functionality as mentioned in the PR objectives.
20-20:Details
❓ Verification inconclusive
Verify @types/express version update for breaking changes.
The @types/express version was updated from 4.17.1 to 4.17.22, which is a significant jump that may include breaking changes or new type definitions.
Please ensure all Express-related TypeScript code still compiles correctly with the updated type definitions.
🏁 Script executed:
#!/bin/bash # Check for TypeScript compilation issues after @types/express update find backend/src -name "*.ts" -exec grep -l "express" {} \; | head -10Length of output: 1200
Ensure Express types still compile after updating @types/express
The@types/expressentry inbackend/package.jsonwas bumped from 4.17.1 → 4.17.22. Please confirm there are no new type errors before merging:
- Run a full type‐check in the backend folder:
cd backend npm run build # or npx tsc --noEmit- Locate every Express import and verify usage sites:
rg "from 'express'" -n backend/src- Fix any reported compilation failures.
backend/src/modules/user/controllers/user.controller.ts (1)
146-151: LGTM! Upload integration with StorageService looks correct.The file upload implementation correctly integrates with the updated StorageService, passing all required parameters including the fileType parameter. This aligns well with the storage service refactoring mentioned in the relevant code snippets.
frontend/src/App.tsx (1)
97-98: LGTM! Clear routing separation for create vs edit modes.The explicit
isEditModeprop makes the component behavior clear and the new/content/edit/:idroute follows REST conventions. This aligns well with theContentEditorcomponent changes that handle both creation and editing workflows.backend/src/app.ts (2)
8-8: Good addition for filesystem path operations.The
pathimport is needed for the static file serving middleware that serves local uploads during development.
64-67:Details
✅ Verification successful
Static file serving for development uploads looks good.
The middleware correctly serves files from the
local_uploadsdirectory at the/local_uploadsroute, enabling access to locally stored files during development. This aligns with the dual-mode storage strategy.Verify that the
local_uploadsdirectory is properly excluded from version control:
🏁 Script executed:
#!/bin/bash # Description: Check if local_uploads directory is ignored in .gitignore # Expected: local_uploads should be listed in .gitignore if [ -f .gitignore ]; then if grep -q "local_uploads" .gitignore; then echo "✅ local_uploads is properly ignored in .gitignore" grep "local_uploads" .gitignore else echo "❌ local_uploads is NOT found in .gitignore - this could lead to committed uploads" fi else echo "❌ .gitignore file not found" fiLength of output: 213
Static file serving and VCS exclusion verified
- The
app.use("/local_uploads", express.static(...))middleware correctly serves files from thelocal_uploadsdirectory.- Confirmed that
local_uploadsis listed in.gitignore, preventing accidental commits of uploaded files.backend/src/app.config.ts (1)
19-27:Details
✅ Verification successful
Enhanced CSP configuration supports image serving functionality.
The Content Security Policy updates are well-configured:
img-srcdirective allowinglocalhost:3000enables serving development imagesdata:support allows inline/base64 imagescross-originresource policy enables frontend access to backend-served imagesVerify the frontend URL configuration matches the CSP allowlist:
🏁 Script executed:
#!/bin/bash # Description: Check if the frontend URL in environment matches the CSP img-src directive # Expected: Frontend URL should be localhost:3000 or match the CSP configuration echo "Checking environment configuration..." if [ -f ".env" ]; then grep -E "(FRONTEND_URL|PORT)" .env || echo "No frontend URL or port found in .env" else echo "No .env file found" fi echo -e "\nChecking if the CSP img-src directive matches expected frontend URLs..." rg -A 5 "img-src" backend/src/app.config.tsLength of output: 540
CSP
img-srcdirective correctly includeshttp://localhost:3000
- Verification complete: the snippet in
backend/src/app.config.tsmatches the intended development URL."img-src": ["'self'", "http://localhost:3000", "data:"],- No
.envfile was found in the repo; if you rely on aFRONTEND_URLvariable in other environments, ensure its value is added to this CSP allowlist before deployment.backend/src/modules/content/controllers/content.controller.ts (3)
36-44: Excellent input validation for file uploads.The validation correctly checks for:
- Existence of
files.thumbnail- Array structure validation
- Presence of at least one file
This prevents runtime errors and provides clear feedback to clients about missing uploads.
51-57: Good file path validation and StorageService integration.The file path validation prevents errors from missing file data. The file object modification with
{ ...file, path: filePath }ensures compatibility with theStorageService.uploadFilemethod that expects apathproperty.
151-153: Consistent file validation in edit method.The validation matches the pattern used in
uploadThumbnailand prevents processing when no file is uploaded during content editing.frontend/src/pages/content/ContentView.tsx (3)
41-41: LGTM! Consistent refactoring approach.The change from destructured
usertoauthobject provides better flexibility and consistency throughout the component.
470-471: Good fix for the navigation path!Adding the leading slash ensures absolute path navigation, preventing potential routing issues.
475-480: Excellent security improvement!The refactored
isCreatorfunction is more secure and reliable by:
- Directly comparing UIDs instead of relying on localStorage (which can be manipulated)
- Adding proper null safety checks
- Using the authenticated user from the auth context
| if (auth.user?.uid) { | ||
| navigate(`/profile/${auth.user.uid}`); | ||
| } else { |
There was a problem hiding this comment.
Add null safety checks for auth.user access.
In several places, auth.user.uid and auth.user.username are accessed without optional chaining, which could cause runtime errors if the user is null.
Apply optional chaining to prevent potential runtime errors:
- navigate(`/profile/${auth.user.uid}`);
+ navigate(`/profile/${auth.user?.uid}`);- userId: auth.user.uid,
- username: auth.user.username,
+ userId: auth.user?.uid,
+ username: auth.user?.username,Also applies to: 267-268, 356-357, 382-383
🤖 Prompt for AI Agents
In frontend/src/pages/content/ContentView.tsx at lines 234-236, 267-268,
356-357, and 382-383, the code accesses auth.user.uid and auth.user.username
without optional chaining, risking runtime errors if auth.user is null. Update
these lines to use optional chaining (e.g., auth.user?.uid and
auth.user?.username) to safely access these properties only when auth.user is
defined.
| const handleSubmit = async () => { | ||
| setError(""); | ||
|
|
||
| if (!title || !content) { | ||
| setError( | ||
| "Please add some content before summarizing using our AI service." | ||
| "Title and content are required, and were not provided. Please try again." | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| setIsSummarizing(true); | ||
| try { | ||
| const response = await fetch(`${apiURL}/api/v1/summarize`, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ input: content }), | ||
| }); | ||
|
|
||
| const data = await response.json(); | ||
| if (!response.ok) { | ||
| setError( | ||
| data.error || | ||
| "Failed to summarize provided content. Please Try again." | ||
| // Prepare content payload | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const newContent: Record<string, any> = { | ||
| creatorUID: auth.user!.uid, | ||
| title, | ||
| content, | ||
| }; | ||
|
|
||
| // Handle thumbnail upload if present | ||
| if (thumbnail) { | ||
| const formData = new FormData(); | ||
| formData.append("thumbnail", thumbnail); | ||
| const thumbRes = await axios.post( | ||
| `${apiURL}/content/uploadThumbnail`, | ||
| formData, | ||
| { | ||
| headers: { "Content-Type": "multipart/form-data" }, | ||
| } | ||
| ); | ||
| newContent.thumbnailUrl = thumbRes.data.url; | ||
| } | ||
|
|
||
| if (!editor) { | ||
| return; | ||
| } | ||
|
|
||
| const formattedSummary = ` | ||
| <div class="summary-container"> | ||
| <h2 class="summary-title">Summary</h2> | ||
| <div class="summary-content"> | ||
| <p>${data.summary.output.replace(/\n/g, "</p><p>")}</p> | ||
| </div> | ||
| </div> | ||
| `; | ||
|
|
||
| editor.commands.setContent(formattedSummary); | ||
| setContent(formattedSummary); | ||
| Cookies.set("content", formattedSummary); | ||
| } catch (error) { | ||
| setError( | ||
| `Failed to summarize provided content: ${ | ||
| (error as Error).message | ||
| }, something went wrong. Please Try again.` | ||
| ); | ||
| } | ||
|
|
||
| setIsSummarizing(false); | ||
| }; | ||
|
|
||
| /** | ||
| * handleSubmit() -> void | ||
| * | ||
| * @description | ||
| * Handles the submission of the content, setting { title, content, thumbnail } | ||
| * respectively amnd redirecting to the Content page. If the title and content | ||
| * are not provided, it will throw an error and set the error state to the current | ||
| * error message based on the error thrown. | ||
| * | ||
| * @returns | ||
| */ | ||
| function handleSubmit() { | ||
| setError(""); | ||
|
|
||
| if (title === "" || content === "") { | ||
| setError( | ||
| "Title and content are required, and were not provided. Please Try again." | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const newContent: Record<string, any> = { | ||
| creatorUID: user!.uid, | ||
| title, | ||
| content, | ||
| }; | ||
|
|
||
| if (thumbnail) { | ||
| const formData = new FormData(); | ||
| formData.append("thumbnail", thumbnail); | ||
|
|
||
| fetch(`${apiURL}/content/uploadThumbnail`, { | ||
| method: "POST", | ||
| body: formData, | ||
| }) | ||
| .then(async (response) => { | ||
| const res = await response.json(); | ||
| const thumbnailUrl = res.url; | ||
| newContent["thumbnailUrl"] = thumbnailUrl; | ||
|
|
||
| return fetch(`${apiURL}/content`, { | ||
| method: "POST", | ||
| // Determine request method and URL | ||
| let response; | ||
| if (isEditMode) { | ||
| const contentId = window.location.pathname.split("/").pop(); | ||
| response = await axios.put( | ||
| `${apiURL}/content/${contentId}`, | ||
| newContent, | ||
| { | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify(newContent), | ||
| }); | ||
| }) | ||
| .then(async (response) => { | ||
| if (response.status === 200 || response.status === 201) { | ||
| Cookies.remove("content"); | ||
| localStorage.removeItem("title"); | ||
| navigate("/"); | ||
| } else { | ||
| setError("Failed to create content. Please Try again."); | ||
| withCredentials: true, | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| console.log(error); | ||
| setError("Failed to create content. Please Try again."); | ||
| }); | ||
| } else { | ||
| fetch(`${apiURL}/content`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify(newContent), | ||
| }) | ||
| .then((response) => response.json()) | ||
| .then(async () => { | ||
| Cookies.remove("content"); | ||
| localStorage.removeItem("title"); | ||
| navigate("/"); | ||
| }) | ||
| .catch((error) => { | ||
| console.log(error); | ||
| setError("Failed to create content. Please Try again."); | ||
| ); | ||
| } else { | ||
| response = await axios.post(`${apiURL}/content`, newContent, { | ||
| headers: { "Content-Type": "application/json" }, | ||
| withCredentials: true, | ||
| }); | ||
| } | ||
|
|
||
| // Notify followers if content creation/update was successful | ||
| if (response.status === 200 || response.status === 201) { | ||
| Cookies.remove("content"); | ||
| localStorage.removeItem("title"); | ||
| navigate("/"); | ||
| } else { | ||
| setError("Failed to save content. Please try again."); | ||
| } | ||
| } catch (err) { | ||
| console.error(err); | ||
| setError("Failed to save content. Please try again."); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add safety checks and improve content ID extraction.
The handleSubmit function has potential runtime issues with null assertions and unchecked property access.
Apply these safety improvements:
- creatorUID: auth.user!.uid,
+ creatorUID: auth.user?.uid || '',Add validation after thumbnail upload:
);
+ if (!thumbRes.data?.url) {
+ throw new Error("Thumbnail upload failed - no URL returned");
+ }
newContent.thumbnailUrl = thumbRes.data.url;Use the suggested useParams approach:
- const contentId = window.location.pathname.split("/").pop();
+ const contentId = id; // from useParamsCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/pages/content/ContentEditor.tsx around lines 132 to 196, improve
safety by replacing the direct use of auth.user! with a check to ensure
auth.user exists before accessing uid. After uploading the thumbnail, validate
that thumbRes.data.url exists before assigning it to newContent.thumbnailUrl.
Replace the extraction of contentId from window.location.pathname with the
useParams hook from react-router to safely and reliably get the content ID.
These changes will prevent runtime errors from null or undefined values and
improve code robustness.
| private static async uploadFileProduction( | ||
| file: File, | ||
| filePath: string, | ||
| fileName: string, | ||
| fileType: string | ||
| ): Promise<{ url: string }> { | ||
| try { | ||
| const fileBuffer = await fs.readFile(file.webkitRelativePath); | ||
| const key = path.posix.join(filePath, fileName); | ||
|
|
||
| await s3.send( | ||
| new PutObjectCommand({ | ||
| Bucket: BUCKET, | ||
| Key: key, | ||
| Body: fileBuffer, | ||
| ContentType: fileType, | ||
| }) | ||
| ); | ||
|
|
||
| // Generate a signed GET URL for the uploaded file | ||
| const url = await getSignedUrl( | ||
| s3, | ||
| new GetObjectCommand({ | ||
| Bucket: BUCKET, | ||
| Key: key, | ||
| }), | ||
| { expiresIn: 3600 } | ||
| ); | ||
|
|
||
| return { url }; | ||
| } catch (error: any) { | ||
| logger.error("S3 upload error:", error); | ||
| throw new Error(`S3 upload failed: ${error.message}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Fix file reading in uploadFileProduction.
The code uses file.webkitRelativePath which is a browser API and doesn't exist in Node.js context.
Fix the file reading logic:
- const fileBuffer = await fs.readFile(file.webkitRelativePath);
+ let fileBuffer: Buffer;
+ if ('filepath' in file && file.filepath) {
+ fileBuffer = await fs.readFile(file.filepath);
+ } else if ('buffer' in file && file.buffer) {
+ fileBuffer = file.buffer;
+ } else {
+ throw new Error("No valid file data found");
+ }Also consider making the signed URL expiration configurable:
- { expiresIn: 3600 }
+ { expiresIn: parseInt(process.env.S3_SIGNED_URL_EXPIRY || '3600') }📝 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.
| private static async uploadFileProduction( | |
| file: File, | |
| filePath: string, | |
| fileName: string, | |
| fileType: string | |
| ): Promise<{ url: string }> { | |
| try { | |
| const fileBuffer = await fs.readFile(file.webkitRelativePath); | |
| const key = path.posix.join(filePath, fileName); | |
| await s3.send( | |
| new PutObjectCommand({ | |
| Bucket: BUCKET, | |
| Key: key, | |
| Body: fileBuffer, | |
| ContentType: fileType, | |
| }) | |
| ); | |
| // Generate a signed GET URL for the uploaded file | |
| const url = await getSignedUrl( | |
| s3, | |
| new GetObjectCommand({ | |
| Bucket: BUCKET, | |
| Key: key, | |
| }), | |
| { expiresIn: 3600 } | |
| ); | |
| return { url }; | |
| } catch (error: any) { | |
| logger.error("S3 upload error:", error); | |
| throw new Error(`S3 upload failed: ${error.message}`); | |
| } | |
| } | |
| private static async uploadFileProduction( | |
| file: File, | |
| filePath: string, | |
| fileName: string, | |
| fileType: string | |
| ): Promise<{ url: string }> { | |
| try { | |
| // Replace browser-only API with Node-friendly file data handling | |
| let fileBuffer: Buffer; | |
| if ('filepath' in file && file.filepath) { | |
| fileBuffer = await fs.readFile(file.filepath); | |
| } else if ('buffer' in file && file.buffer) { | |
| fileBuffer = file.buffer; | |
| } else { | |
| throw new Error("No valid file data found"); | |
| } | |
| const key = path.posix.join(filePath, fileName); | |
| await s3.send( | |
| new PutObjectCommand({ | |
| Bucket: BUCKET, | |
| Key: key, | |
| Body: fileBuffer, | |
| ContentType: fileType, | |
| }) | |
| ); | |
| // Generate a signed GET URL for the uploaded file | |
| const url = await getSignedUrl( | |
| s3, | |
| new GetObjectCommand({ | |
| Bucket: BUCKET, | |
| Key: key, | |
| }), | |
| { expiresIn: parseInt(process.env.S3_SIGNED_URL_EXPIRY || '3600') } | |
| ); | |
| return { url }; | |
| } catch (error: any) { | |
| logger.error("S3 upload error:", error); | |
| throw new Error(`S3 upload failed: ${error.message}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In backend/src/modules/storage/services/storage.service.ts around lines 134 to
168, the method uploadFileProduction incorrectly uses file.webkitRelativePath to
read the file, which is a browser-only property and not available in Node.js.
Replace this with reading the file from the provided filePath or fileName
parameters using Node.js fs module correctly. Additionally, modify the signed
URL generation to accept an expiration time as a configurable parameter instead
of hardcoding 3600 seconds.
| private static async uploadFileDevelopment( | ||
| file: any, | ||
| filePath: string, | ||
| fileName: string, | ||
| fileType: string | ||
| ): Promise<{ url: string }> { | ||
| try { | ||
| const dir = path.join(LOCAL_UPLOAD_DIR, filePath); | ||
| await fs.mkdir(dir, { recursive: true }); | ||
|
|
||
| const snapshot = await uploadBytes(storageRef, fileBuffer, metadata); | ||
| logger.info(` | ||
| File: ${file} | ||
| FileBuffer: ${fileBuffer} | ||
| Path: ${filePath} | ||
| FileName: ${fileName} | ||
| StorageRef: ${storageRef} | ||
| `); | ||
| logger.info("File uploaded successfully."); | ||
|
|
||
| // Get the download URL | ||
| const downloadURL = await getDownloadURL(snapshot.ref); | ||
| logger.info("Download URL:", downloadURL); | ||
|
|
||
| return { url: downloadURL }; | ||
| } catch (error: any) { | ||
| let errorMessage = error.message; | ||
| let fileBuffer: Buffer; | ||
|
|
||
| // Remove "Firebase: " prefix from the error message | ||
| if (errorMessage.startsWith("Firebase: ")) { | ||
| errorMessage = errorMessage.replace("Firebase: ", ""); | ||
| if (file.filepath) { | ||
| fileBuffer = await fs.readFile(file.filepath); | ||
| } else if (file.buffer) { | ||
| fileBuffer = file.buffer; | ||
| } else { | ||
| throw new Error("No valid file data found (no filepath or buffer)."); | ||
| } | ||
| throw new Error(errorMessage); | ||
|
|
||
| fileName = `${fileName}${fileType ? `.${fileType.split("/")[1]}` : ""}`; | ||
|
|
||
| const fullPath = path.join(dir, fileName); | ||
| await fs.writeFile(fullPath, fileBuffer); | ||
| logger.info(`File saved locally at ${fullPath}`); | ||
|
|
||
| // Return the backend-accessible URL | ||
| const url = `http://localhost:3000/local_uploads/${filePath}/${fileName}`; | ||
| return { url }; | ||
| } catch (error: any) { | ||
| logger.error("Local upload error:", error); | ||
| throw new Error(`Local upload failed: ${error.message}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and configuration in uploadFileDevelopment.
Several improvements needed for robustness and flexibility.
- Replace
anytype with proper typing:
- file: any,
+ file: Express.Multer.File | { buffer: Buffer; filepath?: string },- Make the URL configurable:
- const url = `http://localhost:3000/local_uploads/${filePath}/${fileName}`;
+ const baseUrl = process.env.LOCAL_STORAGE_URL || `http://localhost:${process.env.PORT || 3000}`;
+ const url = `${baseUrl}/local_uploads/${filePath}/${fileName}`;- Improve file extension handling:
- fileName = `${fileName}${fileType ? `.${fileType.split("/")[1]}` : ""}`;
+ const ext = fileType ? `.${fileType.split("/").pop()?.split("+")[0]}` : "";
+ fileName = `${fileName}${ext}`;📝 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.
| private static async uploadFileDevelopment( | |
| file: any, | |
| filePath: string, | |
| fileName: string, | |
| fileType: string | |
| ): Promise<{ url: string }> { | |
| try { | |
| const dir = path.join(LOCAL_UPLOAD_DIR, filePath); | |
| await fs.mkdir(dir, { recursive: true }); | |
| const snapshot = await uploadBytes(storageRef, fileBuffer, metadata); | |
| logger.info(` | |
| File: ${file} | |
| FileBuffer: ${fileBuffer} | |
| Path: ${filePath} | |
| FileName: ${fileName} | |
| StorageRef: ${storageRef} | |
| `); | |
| logger.info("File uploaded successfully."); | |
| // Get the download URL | |
| const downloadURL = await getDownloadURL(snapshot.ref); | |
| logger.info("Download URL:", downloadURL); | |
| return { url: downloadURL }; | |
| } catch (error: any) { | |
| let errorMessage = error.message; | |
| let fileBuffer: Buffer; | |
| // Remove "Firebase: " prefix from the error message | |
| if (errorMessage.startsWith("Firebase: ")) { | |
| errorMessage = errorMessage.replace("Firebase: ", ""); | |
| if (file.filepath) { | |
| fileBuffer = await fs.readFile(file.filepath); | |
| } else if (file.buffer) { | |
| fileBuffer = file.buffer; | |
| } else { | |
| throw new Error("No valid file data found (no filepath or buffer)."); | |
| } | |
| throw new Error(errorMessage); | |
| fileName = `${fileName}${fileType ? `.${fileType.split("/")[1]}` : ""}`; | |
| const fullPath = path.join(dir, fileName); | |
| await fs.writeFile(fullPath, fileBuffer); | |
| logger.info(`File saved locally at ${fullPath}`); | |
| // Return the backend-accessible URL | |
| const url = `http://localhost:3000/local_uploads/${filePath}/${fileName}`; | |
| return { url }; | |
| } catch (error: any) { | |
| logger.error("Local upload error:", error); | |
| throw new Error(`Local upload failed: ${error.message}`); | |
| } | |
| } | |
| private static async uploadFileDevelopment( | |
| - file: any, | |
| + file: Express.Multer.File | { buffer: Buffer; filepath?: string }, | |
| filePath: string, | |
| fileName: string, | |
| fileType: string | |
| ): Promise<{ url: string }> { | |
| try { | |
| const dir = path.join(LOCAL_UPLOAD_DIR, filePath); | |
| await fs.mkdir(dir, { recursive: true }); | |
| let fileBuffer: Buffer; | |
| if (file.filepath) { | |
| fileBuffer = await fs.readFile(file.filepath); | |
| } else if (file.buffer) { | |
| fileBuffer = file.buffer; | |
| } else { | |
| throw new Error("No valid file data found (no filepath or buffer)."); | |
| } | |
| - fileName = `${fileName}${fileType ? `.${fileType.split("/")[1]}` : ""}`; | |
| + const ext = fileType | |
| + ? `.${fileType.split("/").pop()?.split("+")[0]}` | |
| + : ""; | |
| + fileName = `${fileName}${ext}`; | |
| const fullPath = path.join(dir, fileName); | |
| await fs.writeFile(fullPath, fileBuffer); | |
| logger.info(`File saved locally at ${fullPath}`); | |
| // Return the backend-accessible URL | |
| - const url = `http://localhost:3000/local_uploads/${filePath}/${fileName}`; | |
| + const baseUrl = | |
| + process.env.LOCAL_STORAGE_URL || | |
| + `http://localhost:${process.env.PORT || 3000}`; | |
| + const url = `${baseUrl}/local_uploads/${filePath}/${fileName}`; | |
| return { url }; | |
| } catch (error: any) { | |
| logger.error("Local upload error:", error); | |
| throw new Error(`Local upload failed: ${error.message}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In backend/src/modules/storage/services/storage.service.ts lines 90 to 123,
replace the 'any' type for the 'file' parameter with a more specific interface
or type that includes 'filepath' and 'buffer' properties to improve type safety.
Make the base URL used in the returned 'url' configurable via environment
variables or configuration files instead of hardcoding 'http://localhost:3000'.
Refactor the file extension extraction logic to handle edge cases more robustly,
such as validating the fileType format before splitting and defaulting to a safe
extension if needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
frontend/src/pages/content/ContentEditor.tsx (1)
131-208:⚠️ Potential issueFix critical syntax errors and code structure issues.
The handleSubmit function has severe syntax errors and incomplete code that will prevent compilation. The static analysis correctly identifies parse errors.
The code mixes axios and fetch inconsistently and has broken syntax. Here's a corrected version:
- const handleSubmit = async () => { - setError(""); - - if (!title || !content) { - setError( - "Title and content are required, and were not provided. Please try again." - ); - return; - } - - try { - // Prepare content payload - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const newContent: Record<string, any> = { - creatorUID: auth.user!.uid, - title, - content, - }; - - // Handle thumbnail upload if present - if (thumbnail) { - const formData = new FormData(); - formData.append("thumbnail", thumbnail); - const thumbRes = await axios.post( - `${apiURL}/content/uploadThumbnail`, - formData, - { - headers: { "Content-Type": "multipart/form-data" }, - } - ); - newContent.thumbnailUrl = thumbRes.data.url; - } - - // Determine request method and URL - let response; - if (isEditMode) { - const contentId = window.location.pathname.split("/").pop(); - response = await axios.put( - `${apiURL}/content/${contentId}`, - newContent, - { - headers: { "Content-Type": "application/json" }, - withCredentials: true, - body: JSON.stringify(newContent), - }); - }) - .then(async (response) => { - if (response.status === 200 || response.status === 201) { - Cookies.remove("content"); - localStorage.removeItem("title"); - navigate("/"); - } else { - setError("Failed to create content. Please Try again."); - } - ); - } else { - response = await axios.post(`${apiURL}/content`, newContent, { - headers: { "Content-Type": "application/json" }, - withCredentials: true, - }); - } else { - fetch(`${apiURL}/content`, { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify(newContent), - }) - .then((response) => response.json()) - .then(async () => { - Cookies.remove("content"); - localStorage.removeItem("title"); - navigate("/"); - }) - .catch((error) => { - console.log(error); - setError("Failed to create content. Please Try again."); - }); - } - }; + const handleSubmit = async () => { + setError(""); + + if (!title || !content) { + setError( + "Title and content are required, and were not provided. Please try again." + ); + return; + } + + if (!auth.user?.uid) { + setError("Authentication required. Please login again."); + return; + } + + try { + // Prepare content payload + const newContent: Record<string, any> = { + creatorUID: auth.user.uid, + title, + content, + }; + + // Handle thumbnail upload if present + if (thumbnail) { + const formData = new FormData(); + formData.append("thumbnail", thumbnail); + const thumbRes = await axios.post( + `${apiURL}/content/uploadThumbnail`, + formData, + { + headers: { "Content-Type": "multipart/form-data" }, + withCredentials: true, + } + ); + + if (!thumbRes.data?.url) { + throw new Error("Thumbnail upload failed - no URL returned"); + } + newContent.thumbnailUrl = thumbRes.data.url; + } + + // Determine request method and URL + let response; + if (isEditMode) { + const contentId = id; // from useParams + response = await axios.put( + `${apiURL}/content/${contentId}`, + newContent, + { + headers: { "Content-Type": "application/json" }, + withCredentials: true, + } + ); + } else { + response = await axios.post(`${apiURL}/content`, newContent, { + headers: { "Content-Type": "application/json" }, + withCredentials: true, + }); + } + + if (response.status === 200 || response.status === 201) { + Cookies.remove("content"); + localStorage.removeItem("title"); + navigate("/"); + } else { + setError(`Failed to ${isEditMode ? 'update' : 'create'} content. Please try again.`); + } + } catch (error) { + console.error(error); + setError(`Failed to ${isEditMode ? 'update' : 'create'} content. Please try again.`); + } + };Key fixes:
- Removed syntax errors and incomplete statements
- Added null safety for
auth.user?.uidas mentioned in past reviews- Used consistent axios calls instead of mixing with fetch
- Added proper error handling and validation
- Used
idfrom useParams instead of URL parsing- Added validation for thumbnail upload response
🧰 Tools
🪛 Biome (1.9.4)
[error] 176-177: Expected a statement but instead found ')
.then(async (response) =>'.Expected a statement here.
(parse)
[error] 185-185: Expected a statement but instead found ')'.
Expected a statement here.
(parse)
[error] 186-186: Expected a statement but instead found 'else'.
Expected a statement here.
(parse)
[error] 191-191: Expected a statement but instead found 'else'.
Expected a statement here.
(parse)
[error] 208-208: Expected a catch clause but instead found ';'.
Expected a catch clause here.
(parse)
[error] 177-186: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.(lint/complexity/noUselessLoneBlockStatements)
[error] 186-191: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.(lint/complexity/noUselessLoneBlockStatements)
[error] 191-207: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.(lint/complexity/noUselessLoneBlockStatements)
♻️ Duplicate comments (1)
frontend/src/pages/content/ContentEditor.tsx (1)
66-98:⚠️ Potential issueAddress the content ID extraction issue mentioned in past reviews.
The code still uses the fragile URL parsing approach that was flagged in previous reviews. Additionally, there's a missing axios import.
Add the missing axios import:
+import axios from 'axios';Use React Router's
useParamshook as suggested in past reviews:-import { useNavigate } from "react-router-dom"; +import { useNavigate, useParams } from "react-router-dom";Then in the component:
+ const { id } = useParams<{ id: string }>();And replace the fragile URL parsing:
- const content = await axios.get( - `${apiURL}/content/${window.location.pathname.split("/").pop()}`, + const content = await axios.get( + `${apiURL}/content/${id}`,
🧹 Nitpick comments (1)
frontend/src/pages/content/ContentEditor.tsx (1)
114-125: Improve error handling for thumbnail changes.Consider clearing the error state when a valid image is selected to provide better user feedback.
const handleThumbnailChange = (e: React.ChangeEvent<HTMLInputElement>) => { const file = e.target.files ? e.target.files[0] : null; if (file && file.type.startsWith("image/")) { setThumbnail(file); setThumbnailPreview(URL.createObjectURL(file)); + setError(""); // Clear any previous errors } else { setThumbnail(null); setThumbnailPreview(null); setError("Please select a valid image file."); } };🧰 Tools
🪛 Biome (1.9.4)
[error] 117-117: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)backend/package.json(2 hunks)backend/src/app.config.ts(1 hunks)backend/src/app.ts(2 hunks)frontend/src/pages/content/ContentEditor.tsx(7 hunks)frontend/src/pages/content/ContentView.tsx(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/package.json
- .gitignore
- backend/src/app.config.ts
- backend/src/app.ts
- frontend/src/pages/content/ContentView.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/src/pages/content/ContentEditor.tsx
[error] 185-185: Expected a statement but instead found ')'.
Expected a statement here.
(parse)
[error] 186-186: Expected a statement but instead found 'else'.
Expected a statement here.
(parse)
[error] 208-208: Expected a catch clause but instead found ';'.
Expected a catch clause here.
(parse)
🔇 Additional comments (2)
frontend/src/pages/content/ContentEditor.tsx (2)
19-19: LGTM! Good addition of theisEditModeprop.The component signature change to accept an
isEditModeboolean prop effectively unifies the creation and editing workflows.
221-221: LGTM! Good dynamic title implementation.The conditional title rendering appropriately reflects whether the user is creating or editing content.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/src/pages/content/ContentEditor.tsx (1)
67-100: Address the fragile content ID extraction and improve safety.This code segment has the same issues flagged in previous reviews that need to be addressed.
The current approach still uses
window.location.pathname.split("/").pop()for content ID extraction, which is fragile and was flagged in previous reviews. You should implement the suggesteduseParamsapproach.Additionally, ensure proper error handling and consider the property name consistency issue mentioned in past comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/pages/content/ContentEditor.tsx(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/src/pages/content/ContentEditor.tsx (2)
frontend/src/scripts/api.ts (1)
apiURL(8-8)backend/src/shared/config/firebase.config.ts (1)
auth(31-31)
🔇 Additional comments (4)
frontend/src/pages/content/ContentEditor.tsx (4)
18-18: Good addition of axios import.The axios import is correctly added to support the new async API calls in the component.
20-20: Well-designed prop interface for mode switching.The component signature change to accept
isEditModeproperly enables the component to handle both creation and editing scenarios.
25-25: Good addition of error state management.Adding error state enables proper user feedback for API failures and validation issues.
227-227: Clean dynamic title implementation.The conditional title rendering properly reflects the component's current mode.
| // If submission was successful, redirect to the content page | ||
| if (!error) { | ||
| // Reset local storage and cookies | ||
| localStorage.removeItem("title"); | ||
| Cookies.remove("content"); | ||
| setTitle(""); | ||
| setContent(""); | ||
|
|
||
| const contentId = isEditMode | ||
| ? window.location.pathname.split("/").pop() | ||
| : response?.data?.id; // Get the new content ID from the response | ||
| if (contentId) { | ||
| navigate(`/content/${contentId}`); | ||
| } else { | ||
| setError("Content ID not found after submission. Please try again."); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // User must be authenticated to create content | ||
| if (!auth.isAuthenticated) { | ||
| navigate("/authentication/login"); | ||
| } | ||
| // User must be authenticated to create content | ||
| if (!auth.isAuthenticated) { | ||
| navigate("/authentication/login"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Fix logic flow and improve error handling.
The current logic has several issues with error handling and authentication placement.
Issues:
- Authentication check should be at the beginning, not after submission
- Error check logic is inverted
- Same fragile URL parsing issue
Apply this fix:
- // If submission was successful, redirect to the content page
- if (!error) {
+ // If submission was successful, redirect to the content page
+ if (response) {
// Reset local storage and cookies
localStorage.removeItem("title");
Cookies.remove("content");
setTitle("");
setContent("");
const contentId = isEditMode
? window.location.pathname.split("/").pop()
: response?.data?.id;
if (contentId) {
navigate(`/content/${contentId}`);
} else {
setError("Content ID not found after submission. Please try again.");
}
}
-
- // User must be authenticated to create content
- if (!auth.isAuthenticated) {
- navigate("/authentication/login");
- }Move the authentication check to the beginning of the function as shown in the previous comment.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/pages/content/ContentEditor.tsx around lines 197 to 219, move
the authentication check to the very start of the function to prevent
unauthorized submissions. Correct the error handling logic so that it properly
checks for errors before proceeding. Replace the fragile URL parsing method with
a more reliable way to obtain the content ID, such as using route parameters or
a safer extraction method. Ensure the flow first verifies authentication, then
handles errors correctly, and finally processes the content ID for navigation.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
frontend/src/pages/content/ContentEditor.tsx (1)
3-3: 🛠️ Refactor suggestionImplement useParams for safer content ID extraction.
Add the useParams import to replace fragile URL parsing used throughout the component.
-import { useNavigate } from "react-router-dom"; +import { useNavigate, useParams } from "react-router-dom";
♻️ Duplicate comments (6)
backend/src/modules/storage/services/storage.service.ts (4)
94-99: Fix type safety issues in development upload method.The
fileparameter still uses theanytype, which was flagged in previous reviews. This reduces type safety and IntelliSense support.Apply this fix for better type safety:
private static async uploadFileDevelopment( - file: any, + file: Express.Multer.File | { buffer: Buffer; filepath?: string }, filePath: string, fileName: string, fileType: string ): Promise<{ url: string }> {
121-121: Make local storage URL configurable.The hardcoded localhost URL was flagged in previous reviews and remains unaddressed. This reduces flexibility across different development environments.
Apply this fix to make the URL configurable:
- const url = `http://localhost:3000/local_uploads/${filePath}/${fileName}`; + const baseUrl = process.env.LOCAL_STORAGE_URL || `http://localhost:${process.env.PORT || 3000}`; + const url = `${baseUrl}/local_uploads/${filePath}/${fileName}`;
145-145: CRITICAL: Fix file reading in production method.The code still uses
file.webkitRelativePathwhich is a browser-only API and doesn't exist in Node.js backend context. This will cause runtime errors in production.Apply this critical fix:
- const fileBuffer = await fs.readFile(file.webkitRelativePath); + let fileBuffer: Buffer; + if ('filepath' in file && file.filepath) { + fileBuffer = await fs.readFile(file.filepath); + } else if ('buffer' in file && file.buffer) { + fileBuffer = file.buffer; + } else { + throw new Error("No valid file data found"); + }
164-164: Make S3 signed URL expiration configurable.The hardcoded 3600 seconds expiration was flagged in previous reviews for lacking configurability.
Apply this improvement:
- { expiresIn: 3600 } + { expiresIn: parseInt(process.env.S3_SIGNED_URL_EXPIRY || '3600') }frontend/src/pages/content/ContentEditor.tsx (2)
144-231: CRITICAL: Fix multiple safety issues in handleSubmit function.This function has several critical issues that were flagged in previous reviews:
- Unsafe auth access:
auth.user!.uidcan cause runtime errors- Authentication check placement: Should be at the beginning
- Missing thumbnail validation: No check if upload succeeded
- Fragile URL parsing: Using
window.location.pathname.split("/").pop()Apply these critical fixes:
const handleSubmit = async () => { + // Check authentication first + if (!auth.isAuthenticated || !auth.user) { + navigate("/authentication/login"); + return; + } + setError(""); if (!title || !content) { setError( "Title and content are required, and were not provided. Please try again." ); return; } const newContent: Record<string, any> = { - creatorUID: auth.user!.uid, + creatorUID: auth.user.uid, title, content, }; let response; try { if (thumbnail) { const formData = new FormData(); formData.append("thumbnail", thumbnail); const thumbRes = await axios.post( `${apiURL}/content/uploadThumbnail`, formData, { headers: { "Content-Type": "multipart/form-data" }, } ); + if (!thumbRes.data?.url) { + throw new Error("Thumbnail upload failed - no URL returned"); + } newContent.thumbnailUrl = thumbRes.data.url; } if (isEditMode) { - const contentId = window.location.pathname.split("/").pop(); + const contentId = id; response = await axios.put( `${apiURL}/content/${contentId}`, newContent, { headers: { "Content-Type": "application/json" }, withCredentials: true, } ); } else { // ... rest of POST logic } } catch (error) { console.error("Error submitting content:", error); setError( "An error occurred while submitting the content. Please try again." ); + return; // Early return on error } // If submission was successful, redirect to the content page - if (!error) { + if (response) { // Reset local storage and cookies localStorage.removeItem("title"); Cookies.remove("content"); setTitle(""); setContent(""); const contentId = isEditMode - ? window.location.pathname.split("/").pop() + ? id : response?.data?.id; if (contentId) { navigate(`/content/${contentId}`); } else { setError("Content ID not found after submission. Please try again."); } } - // User must be authenticated to create content - if (!auth.isAuthenticated) { - navigate("/authentication/login"); - } };
20-20: 🛠️ Refactor suggestionAdd useParams hook for content ID extraction.
Extract the content ID using React Router's useParams instead of parsing window.location.
export default function ContentEditor({ isEditMode }: { isEditMode: boolean }) { + const { id } = useParams<{ id: string }>(); const [title, setTitle] = useState("");
🧹 Nitpick comments (1)
frontend/src/pages/content/ContentEditor.tsx (1)
127-138: Minor: Clear error state on valid thumbnail selection.Consider clearing the error state when a valid thumbnail is selected to improve user experience.
const handleThumbnailChange = (e: React.ChangeEvent<HTMLInputElement>) => { const file = e.target.files ? e.target.files[0] : null; if (file && file.type.startsWith("image/")) { setThumbnail(file); setThumbnailPreview(URL.createObjectURL(file)); + setError(""); // Clear any previous error } else { setThumbnail(null); setThumbnailPreview(null); setError("Please select a valid image file."); } };🧰 Tools
🪛 Biome (1.9.4)
[error] 127-127: expected
,but instead foundconstRemove const
(parse)
[error] 130-130: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/src/modules/storage/services/storage.service.ts(1 hunks)frontend/src/pages/content/ContentEditor.tsx(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/src/pages/content/ContentEditor.tsx (2)
frontend/src/scripts/api.ts (1)
apiURL(8-8)backend/src/shared/config/firebase.config.ts (1)
auth(31-31)
🪛 Biome (1.9.4)
frontend/src/pages/content/ContentEditor.tsx
[error] 68-68: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 69-69: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 73-73: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 80-80: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 82-82: expected , but instead found const
Remove const
(parse)
[error] 82-82: expected , but instead found (
Remove (
(parse)
[error] 87-87: expected , but instead found ;
Remove ;
(parse)
[error] 95-95: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 97-97: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 103-103: Expected a statement but instead found 'catch'.
Expected a statement here.
(parse)
[error] 106-106: Expected a catch clause but instead found ';'.
Expected a catch clause here.
(parse)
[error] 110-110: Expected a statement but instead found ', [isEditMode, editor])'.
Expected a statement here.
(parse)
[error] 71-71: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
[error] 69-69: Shouldn't redeclare 'useNavigate'. Consider to delete it or rename it.
'useNavigate' is defined here:
(lint/suspicious/noRedeclare)
[error] 103-105: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (1)
backend/src/modules/storage/services/storage.service.ts (1)
30-42: LGTM: Environment validation improvements implemented.The environment validation has been properly moved before S3 client initialization and now correctly handles development vs production scenarios. This addresses the previously identified runtime error concerns.
| fileType: string | ||
| ): Promise<{ url: string }> { | ||
| try { | ||
| const fileBuffer = await fs.readFile(file.webkitRelativePath); |
There was a problem hiding this comment.
Coderabbit is correct in the concern for this way of accessing the relative path. Firefox on Android is not supported, which can lead to potential issues down the road with those users. This can be addressed or we can choose to not support Android Firefox.
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores