Завдання Chat App by VladSlob · Pull Request #140 · mate-academy/node_chat

VladSlob · 2025-10-20T10:29:01Z

No description provided.

mateacademy-ai-mentor

Review Summary

I’m requesting changes because there are several critical blockers that will break core functionality (message creation/broadcasting and room joining). Specifically:

In src/controllers/message.controller.js: incorrect import path for User (../models/user.model.js) will cause a runtime module-not-found error; fix to ../models/user.js (lines where the import occurs). Also, the controller treats the service result as a Sequelize instance (newMessage.createdAt, newMessage.text) while the service returns a DTO (author, time, text, roomId). Standardize on a single DTO and ensure emitted messages include roomId so WebSocket broadcasting can route to the correct room.
In src/index.js: the room join validation expects numeric roomId, but your Room model uses UUID strings. Accept string UUIDs, and when broadcasting, use the same DTO fields used by the service/controller (e.g., message.author, message.time, message.text, message.roomId).
In src/routes/rooms.route.js: new express.Router() will throw at runtime. Use express.Router() to instantiate the router.

Please also update message.controller.js history mapping to use the service’s DTO fields (m.author, m.time, m.text, m.roomId) or return the service result directly, and validate inputs/status codes in rooms.controller.js (use 400 for bad input, 404 only when not found, wrap service calls in try/catch). Once these are fixed, the app should correctly let clients join rooms, receive previous messages, and broadcast new messages. Good work on setting up models and core services—after aligning DTOs and fixing these runtime issues, you should be in great shape.

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2025-10-20T10:35:12Z

@@ -0,0 +1,71 @@
+import { EventEmitter } from 'events';
+import { messageService } from '../services/message.service.js';
+import { User } from '../models/user.model.js';


Import path is wrong. The code imports User from ../models/user.model.js, but the actual model file is src/models/user.js (exported as User). This will throw a module-not-found error at runtime; change to ../models/user.js or the correct relative path.

mateacademy-ai-mentor · 2025-10-20T10:35:12Z

+  }
+
+  try {
+    const newMessage = await messageService.createMessageInRoom(


Be careful: messageService.createMessageInRoom (service) returns a mapped DTO (see message.service.js) with fields like author, time, text — not a Sequelize instance. The code below treats newMessage as an instance (newMessage.createdAt, newMessage.text) which will be undefined. Either make the service return the instance or use the DTO fields the service provides.

mateacademy-ai-mentor · 2025-10-20T10:35:12Z

+      roomId,
+    );
+
+    const user = await User.findByPk(userId);


You're fetching the user from DB after creating the message. If you need to verify the user exists, check before creating the message to avoid creating messages for non-existent users. Alternatively, rely on the service to include the author's name so you don't need an additional DB lookup.

mateacademy-ai-mentor · 2025-10-20T10:35:12Z

+    const formattedMessage = {
+      author: user.name,
+      time: newMessage.createdAt,
+      text: newMessage.text,
+    };


The formattedMessage does not include roomId. The WebSocket broadcaster in index.js expects message.roomId to route messages to the right room — omitting it prevents broadcasting to the correct room. Also, time: newMessage.createdAt and text: newMessage.text may be undefined due to the service/controller shape mismatch (see earlier comment).

mateacademy-ai-mentor · 2025-10-20T10:35:12Z

+      text: newMessage.text,
+    };
+
+    messageEmitter.emit('message', formattedMessage);


When emitting the message, ensure you emit a canonical DTO like { author, time, text, roomId } so the emitter and WebSocket layer can broadcast correctly. Right now messageEmitter.emit('message', formattedMessage) emits an object missing roomId and with possibly-missing fields.

mateacademy-ai-mentor · 2025-10-20T10:35:12Z

+    const formattedMessages = messages.map((m) => ({
+      author: m.User?.name || 'Unknown',
+      time: m.createdAt,
+      text: m.text,
+    }));


messageService.getAllMessagesInRoom already returns messages mapped to { author, time, text, roomId }. This mapping here uses m.User?.name and m.createdAt, which don't exist on the mapped objects — resulting in author and time being undefined. Either return raw DB objects from the service and map here, or (simpler) send the service's DTOs directly (res.json(messages)) or map using m.author, m.time, m.text.

mateacademy-ai-mentor · 2025-10-20T10:35:12Z

+  if (!title || !userId) {
+    return res.sendStatus(404);


Returning 404 for missing title or userId is misleading. Use 400 (Bad Request) for missing/invalid input and include a JSON error message to help clients (this endpoint is responsible for creating rooms per task requirements).

mateacademy-ai-mentor · 2025-10-20T10:35:12Z

+  if ((!title && !description) || !roomId) {
+    return res.sendStatus(404);


This validation returns 404 when no title/description or roomId is provided. Use 400 for missing/invalid request data. 404 should be returned only when the target resource is not found after attempting the update. Also separate the checks so you can return clearer errors (e.g., missing params vs not found).

mateacademy-ai-mentor · 2025-10-20T10:35:12Z

+    return res.sendStatus(404);
+  }
+
+  await roomsService.createRoom(title, userId, description);


Calls to the service (create/update/delete) are not wrapped in try/catch. If the service throws (for example, room not found or DB error), the error will bubble and result in an uncontrolled 500. Wrap these awaits in try/catch and return meaningful statuses (e.g., 201/204 on success, 404 if service reports not found, 500 for unexpected errors).

mateacademy-ai-mentor · 2025-10-20T10:35:12Z

+    return res.sendStatus(404);
+  }
+
+  await roomsService.updateRoom(roomId, title, description);


Same as above: await roomsService.updateRoom(...) can throw (service validates id/existence). Add try/catch around this call and translate service errors into appropriate HTTP responses so clients get clear errors.

mateacademy-ai-mentor · 2025-10-20T10:35:16Z

+};
+
+const updateRoom = async (req, res) => {
+  const { roomId } = req.params;


roomId is taken from params but not validated. If your Room model uses UUIDs (as in the repo), validate that roomId is a valid UUID string before calling the service to avoid type/lookup issues and improve error messages.

mateacademy-ai-mentor · 2025-10-20T10:35:16Z

+  description: {
+    type: DataTypes.STRING,
+    allowNull: false,


Consider adding a defaultValue: '' (or allowNull: true) to the description field so the database won't reject inserts created without a description. The service currently supplies description = '', but making the DB column have a default makes the model more robust if other code creates rooms directly.

mateacademy-ai-mentor · 2025-10-20T10:35:16Z

+Room.belongsTo(User);
+User.hasMany(Room);


Be explicit about the foreign key to ensure consistent column naming (userId) across associations and queries — for example Room.belongsTo(User, { foreignKey: 'userId' }) and User.hasMany(Room, { foreignKey: 'userId' }). This removes ambiguity and aligns the model with other code that references userId.

mateacademy-ai-mentor · 2025-10-20T10:35:16Z

+
+    const { roomId } = parsed;
+
+    if (!roomId || typeof roomId !== 'number') {


This rejects non-numeric room IDs. The Room model uses UUID strings, so this check prevents valid room IDs (UUIDs) from being accepted. Remove the numeric-type requirement or accept string UUIDs instead so clients can join rooms correctly.

mateacademy-ai-mentor · 2025-10-20T10:35:16Z

+      rooms[roomId].push(ws);
+    }
+
+    const messages = await messageService.getAllMessagesInRoom(roomId);


When sending previous messages to the newly-joined socket, ensure messageService.getAllMessagesInRoom(roomId) is called with the correct roomId type (string UUID) and handle potential errors (wrap in try/catch) so a thrown error doesn't silently break the connection.

mateacademy-ai-mentor · 2025-10-20T10:35:16Z

+});
+
+messageEmitter.on('message', async (message) => {
+  const { roomId } = message;


You destructure roomId from the emitted message. Ensure the emitter always receives a roomId (e.g., emit { author, time, text, roomId } from the message creation flow), otherwise this check will be falsy and the message will be dropped instead of broadcast to the room.

mateacademy-ai-mentor · 2025-10-20T10:35:16Z

+  }
+
+  const formatted = {
+    author: message.author || message.User?.name || 'Unknown',


The formatted message probes message.User?.name and message.createdAt. This is inconsistent with the service/controller mapping which returns author and time. Standardize on a single DTO (for example message.author and message.time) and use those fields here to avoid undefined author/time values in broadcasts.

mateacademy-ai-mentor · 2025-10-20T10:35:16Z

+  return messages.map((msg) => ({
+    author: msg.user?.name || 'Unknown',


When mapping messages you access the included user as msg.user?.name. With Sequelize the included model is usually attached as msg.User (capitalized) when the model is named User. This likely results in author being 'Unknown'. Use msg.User?.name (or confirm the include alias) and keep the shape consistent with other layers.

mateacademy-ai-mentor · 2025-10-20T10:35:16Z

+  return {
+    author: messageWithUser.user?.name || 'Unknown',


After creating a message you fetch messageWithUser and access messageWithUser.user?.name. For the same reason as above this should likely be messageWithUser.User?.name (or use the actual alias from your model associations). Ensure the service returns a canonical DTO (e.g. { author, time, text, roomId }) that other modules consume without remapping.

mateacademy-ai-mentor · 2025-10-20T10:35:16Z

+import express from 'express';
+import { roomController } from '../controllers/rooms.controller.js';
+
+export const roomsRoute = new express.Router();


express.Router is a factory function, not a constructor. Using new express.Router() will throw at runtime. Change this to export const roomsRoute = express.Router(); so the router is created correctly.

mateacademy-ai-mentor · 2025-10-20T10:35:20Z

+const createRoom = async (title, userId, description = '') => {
+  if (!title || !userId) {
+    throw new Error('Title and userId are required');
+  }
+
+  return Room.create({ title, userId, description });


Consider validating that userId exists before creating a room to avoid creating rooms referencing non-existent users. This improves data integrity and prevents orphaned records.

mateacademy-ai-mentor · 2025-10-20T10:35:20Z

+const deleteRoom = async (id) => {
+  if (!id) {
+    throw new Error('Room id is required');
+  }
+
+  const deletedCount = await Room.destroy({ where: { id } });


The destroy and update calls use id in the where-clause. The Room model uses UUIDs for ids, so make sure callers pass the correct ID type (UUID string). Note this file is fine, but other layers must be consistent with UUID vs numeric IDs.

mateacademy-ai-mentor · 2025-10-20T10:35:20Z

+
+  const [updatedCount] = await Room.update(updatedData, { where: { id } });


Sequelize update returns an array where the first element is the number of affected rows, which you handle correctly. As an improvement, consider returning the updated room object (or fetching it after update) so controllers can return a full resource instead of just the affected count.

mateacademy-ai-mentor

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

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

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.

FaiHamid

Greate job!

Please add the changes suggested by AI after review.
But be careful, if the advice contradicts the conditions of the task, you can ignore it.

VladSlob added 14 commits October 20, 2025 13:23

index.js

36fa326

db.js

542cc18

message.controller.js

446e2d9

rooms.controller.js

52b6863

user.controller.js

1c7dfcb

message.js

ed543fd

room.js

46cead8

user.js

475c4b0

message.route.js

dd4e565

rooms.route.js

c66d602

user.route.js

b82c44b

message.service.js

67f7ab4

rooms.service.js

7ebf747

user.service.js

7fccc5e

mateacademy-ai-mentor suggested changes Oct 20, 2025

View reviewed changes

mateacademy-ai-mentor reviewed Oct 20, 2025

View reviewed changes

FaiHamid suggested changes Oct 22, 2025

View reviewed changes

VladSlob closed this by deleting the head repository Oct 24, 2025

		if ((!title && !description) \|\| !roomId) {
		return res.sendStatus(404);


		const { roomId } = parsed;

		if (!roomId \|\| typeof roomId !== 'number') {

		return messages.map((msg) => ({
		author: msg.user?.name \|\| 'Unknown',


		const [updatedCount] = await Room.update(updatedData, { where: { id } });

		Room.belongsTo(User);
		User.hasMany(Room);

Conversation

VladSlob commented Oct 20, 2025

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Uh oh!

FaiHamid left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants