Minor changes #20
Minor changes #20
Conversation
WalkthroughThis update introduces image deletion and improved image update handling in both project and achievement controllers. When deleting or updating projects or achievements, the associated image is now properly deleted or replaced. Tests are updated to mock and verify image deletion. Additional changes include minor code cleanup and improved API documentation formatting. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Service
participant ImageUtils
%% Project/Achievement Deletion Flow
Client->>Controller: DELETE /projects/:id or /achievements/:id
Controller->>Service: getProjectById(id) or getAchievementById(id)
Service-->>Controller: Project/Achievement (with imageUrl)
alt imageUrl exists
Controller->>ImageUtils: deleteImage(req, imageUrl)
ImageUtils-->>Controller: (image deleted)
end
Controller->>Service: deleteProjectById(id) or deleteAchievementById(id)
Service-->>Controller: (deleted)
Controller-->>Client: 200 OK
%% Project/Achievement Update Flow (with image)
Client->>Controller: PATCH /projects/:id or /achievements/:id (with file)
Controller->>Service: getProjectById(id) or getAchievementById(id)
Service-->>Controller: Project/Achievement (with imageUrl)
Controller->>ImageUtils: uploadImage(req, oldImageUrl)
ImageUtils-->>Controller: newImageUrl
Controller->>Service: updateProjectById(id, newImageUrl, ...)
Service-->>Controller: updated entity
Controller-->>Client: 200 OK
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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: 3
🧹 Nitpick comments (1)
src/routes/projects.ts (1)
15-26: Consider retaining error logging for better debugging.The middleware rename from
parseCreateProjectDatatoparseProjectDatais good for broader applicability. However, removing the error logging on JSON parse failure might make debugging more difficult.Consider adding back error logging while keeping the clean response:
} catch (e) { + console.error('JSON parse error in projectData:', e); return res.status(400).json({ message: 'Invalid JSON in projectData field' }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app.ts(1 hunks)src/controllers/project.controller.ts(4 hunks)src/routes/index.ts(0 hunks)src/routes/projects.ts(3 hunks)src/routes/topics.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/routes/index.ts
🧰 Additional context used
🪛 GitHub Actions: Run Tests
src/controllers/project.controller.ts
[error] 92-92: TypeError: (0 , imageUtils_1.deleteImage) is not a function. This error occurred during the test 'deleteProjectRoutes › should return 200 if project deleted' in tests/Project.test.ts.
🔇 Additional comments (6)
src/app.ts (1)
35-35: LGTM: File size limit increase supports larger image uploads.The increase from 2MB to 5MB aligns with the PR objectives to enhance image handling capabilities. This is a reasonable limit for project images while maintaining security controls.
src/routes/projects.ts (2)
2-2: LGTM: Import formatting improvements.The consistent formatting and spacing of imports improves code readability and maintainability.
Also applies to: 5-13
34-64: LGTM: API documentation formatting improvements.The consistent indentation and formatting of API documentation comments improves readability and maintainability.
src/controllers/project.controller.ts (3)
42-42: LGTM: Fixed deployUrl source path.Correctly changed from
req.body.deployUrltoreq.body.projectData.deployUrlto match the expected data structure.
73-73: LGTM: Simplified validation logic.Removing the redundant
!projectIdcheck is appropriate since it's already validated earlier in the function.
88-94: LGTM: Added image cleanup on project deletion.The logic to delete associated images when removing projects is a good practice for resource management, assuming the
deleteImagefunction works correctly.
| import { Request, Response } from "express"; | ||
| import { ApiError } from "../utils/apiError"; | ||
| import { uploadImage } from "../utils/imageUtils"; | ||
| import { deleteImage, uploadImage } from "../utils/imageUtils"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify deleteImage function export and implementation.
The pipeline failure indicates that deleteImage is not a function. This suggests the function is either not exported from ../utils/imageUtils or doesn't exist.
🏁 Script executed:
#!/bin/bash
# Verify deleteImage function exists and is properly exported
echo "Checking if deleteImage function exists in imageUtils:"
ast-grep --pattern 'export $_ deleteImage'
echo "Checking function declarations:"
ast-grep --pattern 'function deleteImage($_) { $$$ }'
ast-grep --pattern 'const deleteImage = $_'Length of output: 338
Implement and Export deleteImage in src/utils/imageUtils.ts
The import in src/controllers/project.controller.ts is failing at runtime because deleteImage isn’t defined or exported. Please:
- In
src/utils/imageUtils.ts, add and export thedeleteImagefunction. For example:export async function deleteImage(imageUrl: string): Promise<void> { // your deletion logic here }
- Verify that the file path in the controller matches the utils file location.
- Fix the typo on line 64 in the controller: change “File is not exits” → “File does not exist”.
- Rename the
fileUlrvariable tofileUrlfor clarity.
🤖 Prompt for AI Agents
In src/utils/imageUtils.ts around the top of the file, implement and export an
async function named deleteImage that accepts an imageUrl string parameter and
contains the logic to delete the image. Then, in
src/controllers/project.controller.ts, ensure the import path for deleteImage
matches the utils file location exactly. Also, on line 64 of the controller,
correct the typo in the error message from "File is not exits" to "File does not
exist" and rename the variable fileUlr to fileUrl for clarity and correctness.
| if( !projectId ) throw new ApiError("ProjectId is missng !!!" , 401); | ||
|
|
||
| if (file) { | ||
| imageUrl = await uploadImage(supabase, file, 'projects'); | ||
| if ( file ) { | ||
| const response = await projectService.getProjectById(projectId); | ||
| const fileUlr = response?.imageUrl; | ||
| if( !fileUlr ) throw new ApiError("File is not exits"); | ||
| imageUrl = await uploadImage(supabase, file, 'projects' , fileUlr); | ||
| } |
There was a problem hiding this comment.
Fix typos and improve error handling.
Several issues need attention:
- Line 63: Variable name
fileUlrshould befileUrl - Line 64: Error message should be more descriptive
- Missing error status code on line 64
if ( file ) {
const response = await projectService.getProjectById(projectId);
- const fileUlr = response?.imageUrl;
+ const fileUrl = response?.imageUrl;
- if( !fileUlr ) throw new ApiError("File is not exits");
+ if( !fileUrl ) throw new ApiError("Existing image not found", 404);
- imageUrl = await uploadImage(supabase, file, 'projects' , fileUlr);
+ imageUrl = await uploadImage(supabase, file, 'projects' , fileUrl);
}📝 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.
| if( !projectId ) throw new ApiError("ProjectId is missng !!!" , 401); | |
| if (file) { | |
| imageUrl = await uploadImage(supabase, file, 'projects'); | |
| if ( file ) { | |
| const response = await projectService.getProjectById(projectId); | |
| const fileUlr = response?.imageUrl; | |
| if( !fileUlr ) throw new ApiError("File is not exits"); | |
| imageUrl = await uploadImage(supabase, file, 'projects' , fileUlr); | |
| } | |
| if( !projectId ) throw new ApiError("ProjectId is missng !!!" , 401); | |
| if ( file ) { | |
| const response = await projectService.getProjectById(projectId); | |
| const fileUrl = response?.imageUrl; | |
| if( !fileUrl ) throw new ApiError("Existing image not found", 404); | |
| imageUrl = await uploadImage(supabase, file, 'projects' , fileUrl); | |
| } |
🤖 Prompt for AI Agents
In src/controllers/project.controller.ts around lines 59 to 66, correct the typo
by renaming the variable 'fileUlr' to 'fileUrl'. Improve the error message on
line 64 to be more descriptive, such as "File does not exist for the given
project", and add an appropriate HTTP status code (e.g., 404) to the ApiError
thrown there for consistent error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/controllers/achievement.controller.ts (1)
98-121: Excellent logic restructuring with minor formatting issue.The restructured flow is much better:
- Validates existence before processing
- Properly handles image replacement by passing existing URL
- Includes uploaded image in field validation
However, there's an indentation issue that affects readability.
Fix the indentation for better code consistency:
- if ( - !title && - !description && - !achievedAt && - !imageUrl && - (!Array.isArray(memberIds) || memberIds.length === 0) - ) { - throw new ApiError("At least one field must be provided for update", 400); - } + if ( + !title && + !description && + !achievedAt && + !imageUrl && + (!Array.isArray(memberIds) || memberIds.length === 0) + ) { + throw new ApiError("At least one field must be provided for update", 400); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/controllers/achievement.controller.ts(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Run Tests
src/controllers/achievement.controller.ts
[error] 151-151: TypeError: (0 , imageUtils_1.deleteImage) is not a function. This error occurred during the test 'deleteAchievementById › should delete the achievement and return 200 success message' in the test suite 'tests/Achievement.test.ts'.
🔇 Additional comments (1)
src/controllers/achievement.controller.ts (1)
145-153: Excellent addition of proper resource cleanup and validation.The added logic significantly improves the delete operation by:
- Validating achievement existence before deletion (prevents unnecessary operations)
- Properly cleaning up associated images to prevent storage bloat
- Providing appropriate 404 errors for missing resources
This follows best practices for resource management and maintains data consistency.
| import { Request, Response } from "express"; | ||
| import * as achievementService from "../services/achievement.service"; | ||
| import { uploadImage } from "../utils/imageUtils"; | ||
| import { uploadImage, deleteImage } from "../utils/imageUtils"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify deleteImage function exists and is properly exported.
The pipeline failure indicates deleteImage is not a function, suggesting either the function doesn't exist in the imageUtils module or isn't properly exported.
Let me verify the imageUtils implementation:
🏁 Script executed:
#!/bin/bash
# Check if deleteImage function exists and is properly exported
cat src/utils/imageUtils.ts | grep -A 10 -B 2 "deleteImage"
# Also check if there are any export issues
rg "export.*deleteImage" src/utils/Length of output: 447
I want to inspect the full imageUtils.ts to confirm all exports and see if a default export or other export styles might be conflicting with how tests import/mocking it.
🏁 Script executed:
#!/bin/bash
# Display the first 200 lines of imageUtils.ts to verify export patterns
sed -n '1,200p' src/utils/imageUtils.tsLength of output: 2363
Controller import is correct; update your tests’ mocks for deleteImage
The deleteImage function is properly defined and exported in src/utils/imageUtils.ts. The runtime error in your pipeline (“deleteImage is not a function”) indicates that your test suite’s mock of the imageUtils module doesn’t include a deleteImage stub. To resolve:
• Locate your Jest manual mock for src/utils/imageUtils (e.g., in __mocks__/src/utils/imageUtils.ts).
• Add or export a mock implementation for deleteImage, for example:
export const deleteImage = jest.fn(async () => {});• Ensure both uploadImage and deleteImage are exported in that mock.
With your mocks updated, the controller import will work as expected and the tests should pass.
🤖 Prompt for AI Agents
In src/controllers/achievement.controller.ts at line 3, the import of
deleteImage is correct, but your Jest manual mock for src/utils/imageUtils is
missing a mock implementation for deleteImage, causing runtime errors in tests.
To fix this, locate the Jest manual mock file for src/utils/imageUtils (usually
in __mocks__/src/utils/imageUtils.ts) and add an exported mock function for
deleteImage, such as export const deleteImage = jest.fn(async () => {}); also
ensure uploadImage is exported in the mock. This will align the mock with the
actual module exports and resolve the test errors.
increase the size of image as well as the add auto update and delete functionality of storage image
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests