SOlution 1.0#131
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Decision: Request changes — There are multiple critical blockers that break core functionality and violate requirements.
Critical issues to fix:
- src/createServer.js: Root route uses
res.sendStatus(200).json(...)which sends two responses and will throw “Cannot set headers after they are sent.” Change tores.status(200).json({ message: 'Server is running' })(line ~20). - src/services/message.service.js and retrieval path: Messages must include author and be ordered chronologically. Update
findRoomsMessagesto includeUserandorder: [['createdAt','ASC']], and return DTOs like{ author: user.name, text, time: createdAt }. Also changesendMessageto return the created DB record (withcreatedAtand author), not the plain input object. - src/messageServer.js: Calls non-existent
messageService.createMessage(runtime error). Align with the service (sendMessage) or exportcreateMessagefrom the service. Wrap async handlers in try/catch and ensure broadcasts includeauthorandtime. - Room join flow: There is no HTTP endpoint to persist DB membership. Add a route like
POST /:roomId/join(room.router.js) and implementjoinRoomin the controller to addreq.userto the room in the DB; otherwiseisUserInRoomblocks history access for new members. - src/routes/room.router.js: Create route is incorrectly mounted at
POST('/:roomId'). Change toPOST('/')to match controller expectations and avoid confusion. - src/controllers/room.controller.js (mergeRooms): Validate
req.userbefore usinguser.idand normalizetargetRoomId(e.g.,parseInt) before passing to services to avoid crashes/type issues. Remove the redundantif (!roomInfo)check.
Minor improvements (non-blockers): add centralized error-handling middleware; normalize headers in auth middlewares; await Room.create in room.service.createRoom; validate userId in user.controller routes/authorization.
You’ve built a solid structure (models, associations, routers), but resolving the blockers above is necessary for the chat to function (message creation/broadcasting, history with author/time, and joining rooms). Keep going—you’re close.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| const getRoomInfoByRoomId = async (req, res) => { | ||
| const { roomId } = req.params; | ||
| const room = await roomService.findRoomById(roomId); | ||
| const messages = await messageService.findRoomsMessages(roomId); |
There was a problem hiding this comment.
The controller calls messageService.findRoomsMessages(roomId) but the current message service returns raw messages without joining the User (author) and without ordering by creation time. The task requires that messages include author, time and text and that history is returned chronologically. Update message retrieval to include the User association and order by createdAt (ASC), or have the service return mapped objects like { author, text, time } so clients get the author name and timestamp. See message.service.js and message model for context.
|
|
||
| if (!roomInfo) { | ||
| return res.status(400).json({ message: 'Cannot transfer rooms info' }); | ||
| } |
There was a problem hiding this comment.
This check is redundant: roomInfo was just constructed and will always be truthy. Remove the if (!roomInfo) block — it cannot fail and adds unnecessary code. Keeping it may hide real errors elsewhere; instead, rely on earlier checks and propagate actual errors if needed.
| const user = req.user; | ||
| const { targetRoomId } = req.body; | ||
|
|
||
| const usersRooms = await roomService.findRoomsByUserId(user.id); |
There was a problem hiding this comment.
In mergeRooms the code calls roomService.findRoomsByUserId(user.id) before checking whether user exists. If req.user is undefined this will throw. Validate user (and currentRoom) before using user.id or calling services that use it. Reorder the checks so if (!user) appears before any use of user.id.
|
|
||
| if (!user) { | ||
| return res.status(404).json({ error: 'User not found' }); | ||
| } |
There was a problem hiding this comment.
The user existence check happens after usersRooms is used. Move the if (!user) check earlier (before calling findRoomsByUserId) to avoid possible runtime exceptions when req.user is missing.
| await userService.mergeUsers(currentRoom.id, targetRoomId); | ||
| await messageService.mergeMessages(currentRoom.id, targetRoomId); |
There was a problem hiding this comment.
When you call userService.mergeUsers(currentRoom.id, targetRoomId) and messageService.mergeMessages(currentRoom.id, targetRoomId) you pass targetRoomId directly (likely a string). Earlier you parsed it to find targetRoom. Normalize the ID (e.g. parseInt(targetRoomId, 10)) or ensure the services accept string IDs — keep types consistent to avoid subtle bugs.
| for (const user of usersToMerge) { | ||
| if (!targetUserIds.has(user.id)) { | ||
| await targetRoom.addUser(user); | ||
| } | ||
| } | ||
|
|
||
| return { merged: usersToMerge.length, into: targetRoomId }; |
There was a problem hiding this comment.
mergeUsers returns { merged: usersToMerge.length, into: targetRoomId }, which reports the total number of users in the source room rather than how many users were actually added to the target. Instead, track a counter of how many users are newly added (increment when await targetRoom.addUser(user) runs) and return that value so callers get a truthful merged count. This fixes incorrect reporting when some users are already present in the target room.
| const findUsersByRoomId = async (roomId) => { | ||
| const room = await Room.findByPk(roomId, { | ||
| include: User, | ||
| }); |
There was a problem hiding this comment.
Good: findUsersByRoomId uses Room.findByPk(roomId, { include: User }) and returns room.Users. This leverages the model associations correctly to fetch members of a room. Keep this approach — it aligns with the project associations and makes merge logic simpler elsewhere.
| return async function (req, res, next) { | ||
| try { | ||
| await action(req, res, next); |
There was a problem hiding this comment.
This wrapper correctly awaits the async action and forwards errors to the Express error handler via next(error) — this is the correct pattern for async Express controllers and matches how routers wrap controllers in this project (e.g. user and room routers). No change required here.
| } catch (error) { | ||
| next(error); |
There was a problem hiding this comment.
Optional improvement: consider adding conditional logging here (e.g. log in development) before forwarding the error to help debugging. Keep logging behind an environment check so production logs are not noisy.
| export const catchError = (action) => { | ||
| return async function (req, res, next) { | ||
| try { | ||
| await action(req, res, next); |
There was a problem hiding this comment.
Optional: in some test scenarios it can be helpful to return the action's return value (e.g. return await action(...)) so callers can inspect it; not required for Express but worth considering if you introduce tests that call the wrapper directly.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Decision: REQUEST_CHANGES
There are several critical blockers that will break core flows at runtime and prevent the task from being completed reliably.
Critical issues to fix:
- joinRoom passes arguments in the wrong order:
roomService.addUserToRoom(roomId, userId)but the service expects(userId, roomId). Swap toroomService.addUserToRoom(userId, roomId)insrc/controllers/room.controller.js(join handler). This currently prevents persisting membership and blocks accessing history via membership-guarded routes. - Message merge is broken:
messageService.mergeMessagesuses DTOs fromfindRoomsMessagesand then calls.save()on them. Replace logic to operate on Message model instances (e.g.,Message.update({ roomId: target }, { where: { roomId: source } })or fetch instances and save in a transaction). As-is,mergeRoomswill throw at runtime. - Socket message creation mismatch: in
src/messageServer.jsyou callmessageService.createMessage({ roomId, userId, text }), but the service signature is(roomId, userId, text). Change the call toawait messageService.createMessage(roomId, userId, text)(or adjust the service to accept an object). userService.getUserNameByIdis incorrect: wrong where key and missingawait. Implement withconst user = await User.findByPk(userId); return user ? user.name : null;to avoid runtime exceptions.- Route error handling: the join route in
src/routes/room.router.jsis not wrapped withcatchError. Wrap it to ensure async errors are forwarded:roomRouter.post('/:roomId/join', isAuth, catchError(roomController.joinRoom));.
Important improvements (non-blocking but recommended):
- In
getRoomInfoByRoomId, validate the room exists before fetching messages; remove the unreachableif (!roomInfo)check; normalizeroomIdto a number. - Enforce non-empty text for messages in
message.service.createMessageand in the socket handler (trim and validate before saving). - Harden middlewares (
isAuth,isNotAuth,isUserInRoom) with trimming, defensive checks forreq.user, and try/catch that callsnext(err).
You’ve built a solid structure with correct models, associations, and route coverage. Fixing the blockers above will make joining, messaging, and merging reliable. Keep going — you’re close.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| const getRoomInfoByRoomId = async (req, res) => { | ||
| const { roomId } = req.params; | ||
| const room = await roomService.findRoomById(roomId); | ||
| const messages = await messageService.findRoomsMessages(roomId); |
There was a problem hiding this comment.
In getRoomInfoByRoomId you call messageService.findRoomsMessages(roomId) before validating the room. If the room doesn't exist you still made a DB call. Move the room existence check before fetching messages so you only query messages for an existing room. This avoids unnecessary work and clarifies error cases. See the call on this line.
| if (!roomId || !room) { | ||
| return res.status(404).json({ message: 'Room not Found' }); |
There was a problem hiding this comment.
This if (!roomId || !room) check is good (you verify existence) but it should run before any message queries (see previous comment). Also consider normalizing/parsing roomId to an integer consistently if other code expects numeric ids.
| messages, | ||
| }; | ||
|
|
||
| if (!roomInfo) { |
There was a problem hiding this comment.
The if (!roomInfo) check is unreachable: roomInfo is just constructed above and will always be truthy. Remove this redundant check to simplify the flow.
| return res.status(404).json({ message: 'Room not found' }); | ||
| } | ||
|
|
||
| await roomService.addUserToRoom(roomId, userId); |
There was a problem hiding this comment.
Critical runtime bug: joinRoom calls roomService.addUserToRoom(roomId, userId) but addUserToRoom in room.service.js expects the signature (userId, roomId) (userId first) — passing arguments in the wrong order will make the function attempt to find the wrong user/room and fails to persist membership. Swap the arguments to roomService.addUserToRoom(userId, roomId) (or adjust the service signature) to fix joining. See room.service.js for the expected signature and implementation details .
| } | ||
|
|
||
| await roomService.addUserToRoom(roomId, userId); | ||
|
|
There was a problem hiding this comment.
The router currently mounts the join route without the catchError wrapper, so exceptions thrown by joinRoom won't be forwarded to the central error handler. Ensure this route is wrapped consistently (or handle errors inside this function). See router mounting of other routes for examples (they use catchError) .
|
|
||
| if (!room) { | ||
| return []; |
There was a problem hiding this comment.
findUsersByRoomId returns an empty array if the room doesn't exist. This is acceptable, but be explicit in docs or code comments about this behavior so callers understand they won't get an error but an empty list when the room is missing.
| export const catchError = (action) => { | ||
| return async function (req, res, next) { | ||
| try { | ||
| await action(req, res, next); |
There was a problem hiding this comment.
The wrapper awaits the provided action. If any handler relies on this (uncommon for Express handlers but possible), consider invoking the action with the same this context to be safe: await action.call(this, req, res, next); This preserves context for edge cases.
| @@ -0,0 +1,9 @@ | |||
| export const catchError = (action) => { | |||
| return async function (req, res, next) { | |||
There was a problem hiding this comment.
The returned function signature (req, res, next) is correct for wrapping Express route handlers. Note: this helper must be applied where used (controllers are wrapped in routers), but middleware functions are not automatically wrapped. Make sure any async middleware that can throw is either wrapped or implements its own try/catch.
| } catch (error) { | ||
| next(error); |
There was a problem hiding this comment.
Catching the error and calling next(error) is the right approach to forward errors to central error middleware. Optionally you can enrich or log the error here if you want request-scoped logging before forwarding, but that is a project decision rather than a necessity.
| export const catchError = (action) => { | ||
| return async function (req, res, next) { | ||
| try { | ||
| await action(req, res, next); | ||
| } catch (error) { | ||
| next(error); | ||
| } | ||
| }; | ||
| }; |
There was a problem hiding this comment.
No functional problems found. The await handles both async functions and functions that return promises, so no additional branching is required to support typical controller patterns. Keep using this helper for async controllers as shown in routers.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Educational approval: you’ve built a clear server structure with solid separation of concerns (controllers/services), correct Sequelize associations, and thoughtful router/middleware wiring. Nice fixes like the correct addUserToRoom argument order in joinRoom stand out.
Critical improvements to address next:
- Requirement 1.6: in src/controllers/room.controller.js, joinRoom should return history after adding the user; fetch via
await messageService.findRoomsMessages(parsedRoomId)and include in the response. Ensure messages containauthor,text, andtimein a consistent format (ISO). - Robust ID/auth checks: normalize
roomId/userIdwithparseInt/Number.isNaN in room and user controllers/middlewares; verifyreq.userbefore using it (createRoom/join/rename/delete). - Socket layer: in src/messageServer.js fix the call to
messageService.createMessage(use positional args), authenticate the socket (resolve user server-side), validate non-emptytext, and emit room history on join. - Merge/delete consistency: wrap merge operations in a DB transaction and prefer service-level helpers for message deletes; avoid direct
Message.destroyif a service exists. - Small polish: add a global Express error handler (createServer.js), align Socket.IO CORS with Express, trim/validate usernames in controllers/services, and fix
getUserNameByIdto await and query byid.
Overall, you’re very close — address these targeted fixes to fully meet the checklist and harden runtime behavior. Great progress so far — keep it up!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| await roomService.addUserToRoom(userId, roomId); | ||
|
|
||
| return res.status(200).json({ message: 'Joined room successfully' }); |
There was a problem hiding this comment.
Requirement 1.6 (message history retrieval on join) is not fulfilled here. After adding the user to the room you must return the room's previous messages so the client can display history. Consider calling messageService.findRoomsMessages(...) and include the messages in the JSON response (and/or include members). Example flow:
- add user to room
- fetch messages = await messageService.findRoomsMessages(parsedRoomId)
- return { message: 'Joined room successfully', room: { id, name }, messages }
This change is required by the task checklist.
|
|
||
| const room = await roomService.findRoomById(roomId); |
There was a problem hiding this comment.
You're passing req.params.roomId directly into findRoomById. Normalize and validate it first (parseInt + Number.isNaN check) to avoid edge cases when the service expects a number. Also validate the result before proceeding. Example: const id = parseInt(roomId, 10); if (Number.isNaN(id)) return 400... then use id in service calls.
| const user = req.user; | ||
|
|
||
| if (!name) { | ||
| return res.status(400).json({ message: 'Room name is required' }); | ||
| } | ||
|
|
||
| const newRoom = await roomService.createRoom(name); | ||
|
|
||
| if (!newRoom) { | ||
| return res.status(400).json({ message: 'Cannot create the room' }); | ||
| } | ||
|
|
||
| await newRoom.addUser(user); |
There was a problem hiding this comment.
createRoom assumes req.user exists and then does newRoom.addUser(user). Ensure req.user is present (return 401 if not authenticated) and confirm whether addUser expects the user instance or an id. If it expects an id, use newRoom.addUser(user.id). Add an explicit check before calling addUser to avoid runtime errors when req.user is absent.
| const deleteRoom = async (req, res) => { | ||
| const { roomId } = req.params; | ||
| const room = await roomService.findRoomById(roomId); | ||
|
|
||
| if (!roomId || !room) { | ||
| return res.status(404).json({ message: 'Room not Found' }); | ||
| } |
There was a problem hiding this comment.
In deleteRoom you call roomService.findRoomById(roomId) without normalizing the id. Also the existence check uses if (!roomId || !room) which doesn't validate numeric IDs. Parse the roomId and validate with Number.isNaN, then check the room. This avoids ambiguity and possible false positives/negatives.
| } | ||
|
|
||
| await room.setUsers([]); | ||
| await Message.destroy({ where: { roomId: room.id } }); |
There was a problem hiding this comment.
Line performing Message.destroy will remove message rows directly from the model. Prefer using a messageService.deleteMessagesByRoom(room.id) if available so message-related logic stays in one place and any hooks/side-effects are handled consistently. If a service helper doesn't exist, add one or document why direct model access is used.
| const mergeUsers = async (roomId, targetRoomId) => { | ||
| const room = await roomService.findRoomById(roomId); | ||
| const targetRoom = await roomService.findRoomById(targetRoomId); | ||
| const usersToMerge = await findUsersByRoomId(roomId); | ||
| const targetUsers = await findUsersByRoomId(targetRoomId); | ||
|
|
||
| if (!room || !targetRoom) { | ||
| throw new Error('One or both rooms not found'); | ||
| } | ||
|
|
||
| const targetUserIds = new Set(targetUsers.map((u) => u.id)); | ||
|
|
||
| for (const user of usersToMerge) { | ||
| if (!targetUserIds.has(user.id)) { | ||
| await targetRoom.addUser(user); | ||
| } | ||
| } | ||
|
|
||
| return { merged: usersToMerge.length, into: targetRoomId }; |
There was a problem hiding this comment.
mergeUsers has two issues:
- It calls
roomService.findRoomByIdandroomServiceis the exported object — prefer calling a local helper or refactoring to avoid circular import fragility. 2) The returned{ merged: usersToMerge.length }reports the total users in the source room, not the number actually added to the target (some may already exist there). Consider computingaddedCountduring the loop and returning that, and validate inputs earlier (ensure rooms exist before attempting to find users). You may also want to run the adds in a DB transaction for atomicity.
| const getUserNameById = async (userId) => { | ||
| const user = User.findOne({ where: { userId } }); | ||
|
|
||
| return user.name; |
There was a problem hiding this comment.
Critical bug: getUserNameById does not await the DB call and uses the wrong where clause. Current code: const user = User.findOne({ where: { userId } }); return user.name; This will return a Promise and then attempt to access .name, causing a runtime error. Fix it by awaiting and using the correct key or findByPk, e.g.: const user = await User.findByPk(userId); return user ? user.name : null;. This is used by other services and is critical for message author resolution.
| export const catchError = (action) => { | ||
| return async function (req, res, next) { |
There was a problem hiding this comment.
The wrapper properly awaits the action and forwards any error via next(error), which is the intended behavior for async route handlers in Express. Good. Consider adding a short comment above the function explaining it wraps 3-argument middlewares (req, res, next) to improve clarity.
| export const catchError = (action) => { | ||
| return async function (req, res, next) { | ||
| try { | ||
| await action.call(this, req, res, next); |
There was a problem hiding this comment.
You're calling await action.call(this, req, res, next); which preserves context. As a small defensive improvement, you could validate that action is a function before calling it to fail early with a clearer error if misused (e.g., if (typeof action !== 'function') throw new Error('catchError expects a function')). This is optional but helpful during debugging.
| try { | ||
| await action.call(this, req, res, next); | ||
| } catch (error) { | ||
| next(error); |
There was a problem hiding this comment.
The middleware forwards errors with next(error) which relies on the app having a global Express error handler to return consistent JSON to clients. Ensure createServer.js registers an error-handling middleware after routes to handle these errors (log + res.status(...).json({ error: ... })). See createServer.js for where to add it.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
Something went wrong while processing the review.
[CODE: GAE]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
The test task has already been approved.
[CODE: AAP]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
No description provided.