Refactor Task API, Add Real-time Notifications, and Enhance Frontend #34
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR refactors the task API by moving all CRUD logic inline into the Fastify route definitions, replacing the TaskController, and enriching operations with detailed validation, transaction-based persistence (Mongoose sessions), and socket.io real-time notifications. It also adjusts front-end profile components, improves error handling in the RoomProvider, and includes minor cleanups in user and room controllers. Sequence diagram for task creation with validation, transaction, and notificationssequenceDiagram
actor User
participant Fastify
participant Task
participant UserModel
participant RoomModel
participant SocketIO
User->>Fastify: POST /add/tasks
Fastify->>UserModel: Validate assignees
Fastify->>RoomModel: Validate room access
Fastify->>Task: Create new task (with validated data)
Fastify->>UserModel: Update myTasks, increment tasksCreated
Fastify->>UserModel: Update collaborators for assignees
Fastify->>RoomModel: Add task to room (if roomId)
Fastify->>SocketIO: emitTaskNotification("taskCreated")
Fastify->>SocketIO: emitTaskNotification("taskAssigned") for each assignee
Fastify-->>User: Return saved task
Sequence diagram for task update with validation and notificationssequenceDiagram
actor User
participant Fastify
participant Task
participant UserModel
participant SocketIO
User->>Fastify: PUT /update/tasks/:id
Fastify->>Task: Validate task ownership
Fastify->>Task: Update task fields
Fastify->>UserModel: Increment tasksCompleted (if status changed to done)
Fastify->>SocketIO: emitTaskNotification("taskUpdated")
Fastify->>SocketIO: io.to(`task_${id}`).emit("taskRoomUpdate")
Fastify-->>User: Return updated task
Sequence diagram for task deletion with notificationssequenceDiagram
actor User
participant Fastify
participant Task
participant UserModel
participant SocketIO
User->>Fastify: DELETE /delete/task/:id
Fastify->>Task: Validate task ownership
Fastify->>Task: Delete task
Fastify->>UserModel: Remove task from myTasks
Fastify->>SocketIO: emitTaskNotification("taskDeleted")
Fastify->>SocketIO: io.to(`task_${id}`).emit("taskRoomDeleted")
Fastify-->>User: Return success
Class diagram for removal of TaskController and inline route logicclassDiagram
class TaskController {
-emitTaskNotification()
-getTask()
-addTask()
-updateTask()
-getById()
-delete()
}
%% TaskController is removed; logic now in Fastify route definitions
class FastifyTaskRoutes {
+emitTaskNotification()
+getTask()
+addTask()
+updateTask()
+getById()
+delete()
}
FastifyTaskRoutes : uses Mongoose models
FastifyTaskRoutes : uses socket.io for notifications
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ Deploy Preview for colab-flow ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Owner
Author
|
@sourcery-ai title |
sourcery-ai
bot
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The task routes are very large and repetitive—consider extracting shared validation and notification logic into reusable helper functions or middleware to improve readability and maintainability.
- It may be better to move the inlined controller logic back into dedicated controller/service classes (or at least separate modules) so that your route definitions stay concise and your business logic remains easily testable.
- The front-end component rename (Profile → Profiles) is inconsistent—pick a single name and update imports/exports accordingly to avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The task routes are very large and repetitive—consider extracting shared validation and notification logic into reusable helper functions or middleware to improve readability and maintainability.
- It may be better to move the inlined controller logic back into dedicated controller/service classes (or at least separate modules) so that your route definitions stay concise and your business logic remains easily testable.
- The front-end component rename (Profile → Profiles) is inconsistent—pick a single name and update imports/exports accordingly to avoid confusion.
## Individual Comments
### Comment 1
<location> `src/routes/task.route.js:156` </location>
<code_context>
- }
- }
-
- if (!estimatedHours || typeof estimatedHours !== "number") {
- return reply.code(400).send({
- success: false,
</code_context>
<issue_to_address>
Estimated hours validation may reject valid zero values.
The validation treats 0 as invalid due to its falsy value. Use a type check or null/undefined check to permit zero.
</issue_to_address>
### Comment 2
<location> `src/routes/task.route.js:164` </location>
<code_context>
- }
-
- // Chercher les utilisateurs assignés existants (sans doublon)
- const assigneesId = [...new Set(assignees)];
- // Chercher les utilisateurs assignés existants (sans doublon)
- const existingAssignees = await User.find({
- $and: [{ _id: { $in: assigneesId } }, { _id: { $ne: userId } }],
- }).select("_id");
</code_context>
<issue_to_address>
Duplicate assignee filtering may be redundant.
Using the original assignees array for updates and notifications may cause duplicate actions for users. Use the deduplicated list to avoid this issue.
Suggested implementation:
```javascript
assignees: assigneesId,
```
If there are other places in this function or file where the original `assignees` array is used for notifications, updates, or other actions, you should replace those usages with `assigneesId` to ensure deduplication throughout the workflow.
</issue_to_address>
### Comment 3
<location> `src/routes/task.route.js:178` </location>
<code_context>
- priority,
- dueDate: dueDateObj,
- estimatedHours: parseFloat(estimatedHours) || 0,
- assignees: [...existingAssignees],
- label,
- author: request.user.userId,
</code_context>
<issue_to_address>
Assignees array may contain Mongoose documents instead of IDs.
If the schema requires ObjectIds, map `existingAssignees` to their `_id` properties before assigning.
</issue_to_address>
### Comment 4
<location> `src/routes/task.route.js:343` </location>
<code_context>
- if (updateData.author) delete updateData.author;
- if (updateData.room) delete updateData.room;
-
- const updatedTask = await Task.findByIdAndUpdate(id, updateData, {
- new: true,
- runValidators: true,
</code_context>
<issue_to_address>
Update allows arbitrary fields to be set.
Whitelisting permitted fields for updates will help prevent unauthorized modifications to sensitive data.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// Mettre à jour la tâche
const updateData = { ...request.body };
// Ne pas permettre la modification de l'auteur
if (updateData.author) delete updateData.author;
if (updateData.room) delete updateData.room;
const updatedTask = await Task.findByIdAndUpdate(id, updateData, {
new: true,
runValidators: true,
}).populate("author", "userName email");
=======
// Définir les champs autorisés à être mis à jour
const allowedFields = [
"title",
"description",
"status",
"estimatedTime",
"priority",
"dueDate"
// Ajoutez ici d'autres champs autorisés si nécessaire
];
// Filtrer les données de mise à jour pour ne garder que les champs autorisés
const updateData = {};
for (const key of allowedFields) {
if (request.body.hasOwnProperty(key)) {
updateData[key] = request.body[key];
}
}
const updatedTask = await Task.findByIdAndUpdate(id, updateData, {
new: true,
runValidators: true,
}).populate("author", "userName email");
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| } | ||
|
|
||
| if (!estimatedHours || typeof estimatedHours !== "number") { |
There was a problem hiding this comment.
issue (bug_risk): Estimated hours validation may reject valid zero values.
The validation treats 0 as invalid due to its falsy value. Use a type check or null/undefined check to permit zero.
Comment on lines
+164
to
+166
| // Chercher les utilisateurs assignés existants (sans doublon) | ||
| const existingAssignees = await User.find({ | ||
| $and: [{ _id: { $in: assigneesId } }, { _id: { $ne: userId } }], |
There was a problem hiding this comment.
suggestion (bug_risk): Duplicate assignee filtering may be redundant.
Using the original assignees array for updates and notifications may cause duplicate actions for users. Use the deduplicated list to avoid this issue.
Suggested implementation:
assignees: assigneesId,If there are other places in this function or file where the original assignees array is used for notifications, updates, or other actions, you should replace those usages with assigneesId to ensure deduplication throughout the workflow.
| dueDate: dueDateObj, | ||
| estimatedHours: parseFloat(estimatedHours) || 0, | ||
| assignees: [...existingAssignees], | ||
| label, |
There was a problem hiding this comment.
issue (bug_risk): Assignees array may contain Mongoose documents instead of IDs.
If the schema requires ObjectIds, map existingAssignees to their _id properties before assigning.
Comment on lines
+337
to
+346
| const updateData = { ...request.body }; | ||
| // Ne pas permettre la modification de l'auteur | ||
| if (updateData.author) delete updateData.author; | ||
| if (updateData.room) delete updateData.room; | ||
|
|
||
| const updatedTask = await Task.findByIdAndUpdate(id, updateData, { | ||
| new: true, | ||
| runValidators: true, | ||
| }).populate("author", "userName email"); | ||
|
|
There was a problem hiding this comment.
🚨 suggestion (security): Update allows arbitrary fields to be set.
Whitelisting permitted fields for updates will help prevent unauthorized modifications to sensitive data.
Suggested change
| // Mettre à jour la tâche | |
| const updateData = { ...request.body }; | |
| // Ne pas permettre la modification de l'auteur | |
| if (updateData.author) delete updateData.author; | |
| if (updateData.room) delete updateData.room; | |
| const updatedTask = await Task.findByIdAndUpdate(id, updateData, { | |
| new: true, | |
| runValidators: true, | |
| }).populate("author", "userName email"); | |
| // Définir les champs autorisés à être mis à jour | |
| const allowedFields = [ | |
| "title", | |
| "description", | |
| "status", | |
| "estimatedTime", | |
| "priority", | |
| "dueDate" | |
| // Ajoutez ici d'autres champs autorisés si nécessaire | |
| ]; | |
| // Filtrer les données de mise à jour pour ne garder que les champs autorisés | |
| const updateData = {}; | |
| for (const key of allowedFields) { | |
| if (request.body.hasOwnProperty(key)) { | |
| updateData[key] = request.body[key]; | |
| } | |
| } | |
| const updatedTask = await Task.findByIdAndUpdate(id, updateData, { | |
| new: true, | |
| runValidators: true, | |
| }).populate("author", "userName email"); |
| } = request.body; | ||
|
|
||
| const userId = request.user.userId; | ||
|
|
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
Suggested change
| const userId = request.user.userId; | |
| const {userId} = request.user; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
| try { | ||
| const { id } = request.params; | ||
| const userId = request.user.userId; | ||
|
|
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
Suggested change
| const userId = request.user.userId; | |
| const {userId} = request.user; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
| try { | ||
| const { id } = request.params; | ||
| const userId = request.user.userId; | ||
|
|
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
Suggested change
| const userId = request.user.userId; | |
| const {userId} = request.user; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Refactor task API endpoints to inline Fastify route handlers with direct Mongoose operations, comprehensive validation, transaction support, and real-time WebSocket notifications; clean up controller files; and update frontend profile and room context components for improved loading and error handling.
New Features:
Enhancements:
Chores: