Solution by DSemeniukV · Pull Request #137 · mate-academy/node_chat

DSemeniukV · 2025-10-17T11:50:27Z

No description provided.

mateacademy-ai-mentor

Review Summary

I am requesting changes because core requirements are not met: messages delivered to HTTP and WebSocket clients do not include the required author (username), time, and text fields, and inputs aren’t validated robustly. This breaks the primary functionality where users must see messages with the author’s username.

Critical issues to fix:

In src/controllers/message.controller.js (POST/GET), you emit and return raw Sequelize Message instances without joining User. Include User during create/fetch (e.g., include: [{ model: User, attributes: ['name'] }]), convert to plain objects (toJSON()), and map to { author: user.name, time: createdAt, text } before emitting/returning. Also validate roomId and return 400 for missing params instead of 404.
In src/services/message.service.js, both createMessageInRoom and getAllMessagesInRoom return instances without User. Import User, include it in queries, and normalize returns to plain { author, time, text } consistently.
In src/index.js, the WS join handler and broadcaster send raw model instances and don’t validate incoming payloads. Wrap JSON.parse in try/catch, validate roomId, prevent duplicate socket entries, include User when loading previous messages, and broadcast a mapped plain object containing author, time, text.

Minor but important improvements:

In src/controllers/user.controller.js, validate name before any DB call and return 400 when missing; handle a possible race by catching SequelizeUniqueConstraintError and returning 409. In src/models/message.js, consider DataTypes.TEXT for text and enforce non-null FKs for userId/roomId.

You’ve implemented solid room CRUD and the overall server/WS structure—once the above are addressed, the app will align with the requirements. Keep going—these are targeted changes that will make the system robust and correct.

✨ 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-17T11:56:32Z

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


Returning 404 for missing fields is acceptable but unusual; prefer 400 (Bad Request) for missing parameters. Also ensure you validate the input explicitly for clearer client errors.

mateacademy-ai-mentor · 2025-10-17T11:56:32Z

+  const newMessage = await messageService.createMessageInRoom(
+    text,
+    userId,
+    roomId,
+  );


messageService.createMessageInRoom returns a Message instance containing userId but not the user's name. To satisfy the requirement that each message includes an author username, include the User when creating or immediately fetch the User (e.g., use include: User or query User by userId) and attach author before continuing.

mateacademy-ai-mentor · 2025-10-17T11:56:32Z

+    roomId,
+  );
+
+  messageEmitter.emit('message', newMessage);


Emitting the raw Sequelize instance (newMessage) means the emitted object won't necessarily contain author (username) and may not serialize as expected. Convert to a plain object (newMessage.toJSON() or get({ plain: true })) and emit an object shaped like { author: user.name, time: newMessage.createdAt, text: newMessage.text, ... } so WebSocket clients receive the required fields.

mateacademy-ai-mentor · 2025-10-17T11:56:33Z

+
+  messageEmitter.emit('message', newMessage);
+
+  res.status(201).json(newMessage);


Responding with res.json(newMessage) returns the raw instance. Instead, return a mapped plain object that contains author (username), time (createdAt) and text — this ensures HTTP clients also receive the required shape.

mateacademy-ai-mentor · 2025-10-17T11:56:33Z

+export const getMessages = async (req, res) => {
+  const { roomId } = req.params;
+  const messages = await messageService.getAllMessagesInRoom(roomId);


getMessages delegates to messageService.getAllMessagesInRoom which uses Message.findAll({ where: { roomId } }) and does not include User. This will return messages without authors' names. Include the User in the query (or map messages after fetching by querying users) and return an array of plain objects { author, time, text } so new users joining a room see previous messages with usernames as required.

mateacademy-ai-mentor · 2025-10-17T11:56:33Z

+
+const create = async (req, res) => {
+  const { name } = req.body;
+  const isNameExist = await User.findOne({ where: { name } });


You call User.findOne before validating name. If name is missing this triggers an unnecessary DB query and may produce unexpected results. Move the if (!name) check before this DB lookup so you validate input first.

mateacademy-ai-mentor · 2025-10-17T11:56:33Z

+  if (!name) {
+    return res.sendStatus(404);


Returning 404 for a missing name is misleading — 400 Bad Request is the conventional status for required-missing fields. Consider returning res.sendStatus(400) (and document expected request body). This aligns better with API semantics and the requirement that a username must be provided.

mateacademy-ai-mentor · 2025-10-17T11:56:33Z

+    return res.status(409).json({ message: 'User already exists' });
+  }
+
+  await userService.createUser(name);


await userService.createUser(name) is not protected against a race where another request inserts the same name after your findOne check. Wrap the create call in try/catch and handle the DB unique-constraint error (SequelizeUniqueConstraintError) to return 409 when a duplicate appears. This ensures you reliably return the correct status instead of bubbling an exception.

mateacademy-ai-mentor · 2025-10-17T11:56:33Z

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


The text attribute uses DataTypes.STRING, which may be too short for chat messages and could lead to truncation. Consider using DataTypes.TEXT to avoid length limits so message text is not lost.

mateacademy-ai-mentor · 2025-10-17T11:56:33Z

+    createdAt: {
+      type: DataTypes.DATE,
+      defaultValue: DataTypes.NOW,
+      allowNull: false,
+    },


You define createdAt manually which provides the required timestamp. Alternatively you could enable Sequelize timestamps and keep updatedAt: false to let Sequelize manage createdAt automatically — either approach is fine as long as controllers return this value as the message time.

mateacademy-ai-mentor · 2025-10-17T11:56:37Z

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


Associations Message.belongsTo(User) and Message.belongsTo(Room) create foreign keys but do not enforce NOT NULL. To ensure every message has an author and a room (required by the task), add explicit userId and roomId fields with allowNull: false or specify { foreignKey: { allowNull: false } } in the belongsTo calls.

mateacademy-ai-mentor · 2025-10-17T11:56:37Z

+
+wss.on('connection', (ws) => {
+  ws.on('message', async (message) => {
+    const { roomId } = JSON.parse(message);


JSON.parse(message) can throw and you must validate the parsed payload. Wrap parsing in try/catch and ensure the payload contains a valid roomId. Without this, malformed input will crash the WS handler or create rooms[undefined] which leaks state.

mateacademy-ai-mentor · 2025-10-17T11:56:37Z

+    if (!rooms[roomId]) {
+      rooms[roomId] = [];


You create the room array if missing but never validate roomId before using it; this can create rooms[undefined]. Ensure roomId exists and is a valid identifier before mutating rooms.

mateacademy-ai-mentor · 2025-10-17T11:56:37Z

+      rooms[roomId] = [];
+    }
+
+    rooms[roomId].push(ws);


Pushing ws into rooms[roomId] unconditionally permits duplicate entries if the client sends join multiple times. Prevent duplicates (e.g., check includes, use a Set, or track joined rooms on the socket) so the same socket isn't added repeatedly.

mateacademy-ai-mentor · 2025-10-17T11:56:37Z

+
+    rooms[roomId].push(ws);
+
+    const messages = await Message.findAll({ where: { roomId } });


You fetch messages with Message.findAll({ where: { roomId } }) but don't include the associated User. To satisfy the requirement that messages include the author's username, include the User in the query (or fetch the user separately) so you can send author (user.name) to clients.

mateacademy-ai-mentor · 2025-10-17T11:56:37Z

+    const messages = await Message.findAll({ where: { roomId } });
+
+    messages.forEach((msg) => {
+      ws.send(JSON.stringify(msg));


Sending Sequelize model instances with JSON.stringify(msg) may not produce the intended payload. Convert to a plain object (msg.toJSON() or msg.get({ plain: true })) and map to { author, time: createdAt, text } before sending so clients always receive the required fields.

mateacademy-ai-mentor · 2025-10-17T11:56:37Z

+messageEmitter.on('message', async (message) => {
+  const { roomId } = message;
+
+  if (rooms[roomId]) {
+    rooms[roomId].forEach((client) => {
+      if (client.readyState === 1) {
+        client.send(JSON.stringify(message));


The messageEmitter handler broadcasts the raw message object. That object is likely a Sequelize instance without the joined User name. Before broadcasting, construct a plain payload that includes author (username), time and text so WS clients receive the correctly shaped message.

mateacademy-ai-mentor · 2025-10-17T11:56:38Z

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


description is allowNull: false but has no defaultValue. The rooms service provides a default empty string when creating rooms, so creation works, but consider adding defaultValue: '' here (or allowNull: true) to keep the model and service aligned and avoid potential errors elsewhere.

mateacademy-ai-mentor · 2025-10-17T11:56:38Z

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


This model and its belongsTo/hasMany associations satisfy the requirement to associate rooms with users and therefore support room ownership and room-related operations required by the task (create/rename/delete/join) — ensure other modules use these associations consistently when fetching or serializing rooms.

mateacademy-ai-mentor · 2025-10-17T11:56:38Z

@@ -0,0 +1,14 @@
+import { Message } from '../models/message.js';


The service imports only Message. To return messages with the author's username you should also import the User model (Message belongsTo User) and include it in queries so you can access the user's name to use as author.

mateacademy-ai-mentor · 2025-10-17T11:56:42Z

+const getAllMessagesInRoom = (roomId) => {
+  return Message.findAll({ where: { roomId } });


This returns raw Message instances without the associated User. Use Message.findAll({ where: { roomId }, include: [{ model: User, attributes: ['name'] }], order: [['createdAt','ASC']] }) (or include then map) and convert each result to a plain object like { author: user.name, time: createdAt, text } to satisfy the requirement that previous messages include author, time and text.

mateacademy-ai-mentor · 2025-10-17T11:56:42Z

+const createMessageInRoom = (text, userId, roomId) => {
+  return Message.create({ text, userId, roomId });


Creating the message with Message.create returns an instance that only contains userId, not the user's name. After creation, fetch the created message with the User included (e.g., Message.findByPk(newMsg.id, { include: User })) or perform a join so you can return a plain object { author, time, text }. This ensures callers (controller/emitter) can send messages that include the author's username.

mateacademy-ai-mentor · 2025-10-17T11:56:42Z

+export const messageService = {
+  getAllMessagesInRoom,
+  createMessageInRoom,


Consider normalizing the returned value from both functions to a consistent plain payload (not Sequelize instances). Controllers and WebSocket code assume they can serialize message; returning a plain object avoids surprises and makes broadcasting predictable.

mateacademy-ai-mentor

Review Summary

I’m requesting changes because there are multiple critical blockers that will break core functionality and prevent meeting the requirements.

Critical issues to fix:

src/controllers/message.controller.js: Wrong import path for User (no ../models/user.model.js). Use the actual model path (e.g., ../models/user.js) or User usages will throw at runtime.
src/controllers/message.controller.js: The controller treats the service return as a Sequelize instance (accessing newMessage.createdAt/newMessage.User). The service returns { author, time, text }, so use those fields directly or change the service to return the instance with include: User. Also remove the redundant User.findByPk(userId) if the service already supplies author.
Emitted message payloads are missing roomId in both the controller and service. Add roomId to the object you emit/return so the WebSocket layer can route messages correctly.
src/index.js: The WS join validation requires numeric roomId, but room IDs are UUID strings. Accept non-empty strings (ideally validate UUID) or clients can’t join rooms. Also ensure the WS emitter/consumer consistently use a single shape { author, time, text, roomId }.
src/services/message.service.js: Wrong User import path and createMessageInRoom doesn’t include roomId in its mapped return; include it so emitters/controllers can route messages.

Minor improvements (non-blocking): strengthen input validation and HTTP statuses in room and user controllers (use 400 for bad input, handle SequelizeUniqueConstraintError, trim name), consider TEXT for message text, and define non-null FKs (userId, roomId) in the Message model. Great work on setting up routes and normalizing messages in the service—once the above blockers are addressed, the app should meet the requirements.

✨ 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-17T12:14:25Z

@@ -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 incorrect; there is no ../models/user.model.js file. Import the actual User model (e.g. ../models/user.js) so User.findByPk/Sequelize won't fail at runtime.

mateacademy-ai-mentor · 2025-10-17T12:14:25Z

+    const newMessage = await messageService.createMessageInRoom(
+      text,
+      userId,
+      roomId,
+    );


You call the service to create a message — make sure you handle the exact shape the service returns. If the service returns a mapped object ({ author, time, text }), you must not treat newMessage as a Sequelize instance. Either have the service return the instance with included User, or consume the service's mapped shape here.

mateacademy-ai-mentor · 2025-10-17T12:14:25Z

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


This User.findByPk(userId) is redundant if createMessageInRoom already returns the author (name). Extra DB calls are unnecessary and can be removed — instead rely on the service to provide author/time/text, or make the service return the created instance with included User so you can map once.

mateacademy-ai-mentor · 2025-10-17T12:14:25Z

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


You're building formattedMessage using newMessage.createdAt and newMessage.text, but the service returns time/text/author (not createdAt) — createdAt will be undefined. Use the service's returned time/author/text fields (or change the service to return a raw instance and include User).

mateacademy-ai-mentor · 2025-10-17T12:14:25Z

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


When emitting the message you must include roomId in the payload (e.g. { author, time, text, roomId }). index.js expects message.roomId to route the event to the correct room; without it the WS broadcast will be ignored.

mateacademy-ai-mentor · 2025-10-17T12:14:25Z

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


messageService.getAllMessagesInRoom already maps messages to { author, time, text }. Here you re-map expecting m.User and m.createdAt which will produce incorrect/empty values. Return/send the messages array directly (or align service/controller to a single consistent shape).

mateacademy-ai-mentor · 2025-10-17T12:14:26Z

@@ -0,0 +1,10 @@
+import 'dotenv/config';


Importing dotenv here is fine to load environment variables. Ensure you have a .env in development and that CI/production provide these env vars; otherwise Sequelize will be constructed with undefined values.

mateacademy-ai-mentor · 2025-10-17T12:14:26Z

+import 'dotenv/config';
+import { Sequelize } from 'sequelize';
+
+export const client = new Sequelize({


Sequelize constructor uses several environment variables. Consider validating these values (or providing sensible defaults) before constructing Sequelize so the app fails fast with a clear error when DB config is missing.

mateacademy-ai-mentor · 2025-10-17T12:14:26Z

+import { Sequelize } from 'sequelize';
+
+export const client = new Sequelize({
+  host: process.env.DB_HOST,


If your database uses a non-default port, include port: process.env.DB_PORT (or Number(process.env.DB_PORT) || 5432) in this config. Without it, connections to nonstandard ports will fail.

mateacademy-ai-mentor · 2025-10-17T12:14:26Z

+  username: process.env.DB_USERNAME,
+  password: process.env.DB_PASS,
+  database: process.env.DB_DATABASE,
+  dialect: 'postgres',


For production (e.g. Heroku, managed Postgres) you may need to pass dialectOptions (SSL) or disable verbose logging: e.g. { dialect: 'postgres', logging: false, dialectOptions: { ssl: { rejectUnauthorized: false } } }. Add those only if required by your deployment.

mateacademy-ai-mentor · 2025-10-17T12:14:30Z

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


For missing required parameters you should return 400 (Bad Request), not 404. Change this to res.status(400).json(...) or res.sendStatus(400) so clients get the correct semantics when title or userId are missing.

mateacademy-ai-mentor · 2025-10-17T12:14:30Z

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


Same issue: updateRoom returns 404 for missing roomId or when neither title nor description is provided. Return 400 for invalid/missing input instead of 404 (404 means "not found").

mateacademy-ai-mentor · 2025-10-17T12:14:30Z

+const getAllRooms = async (req, res) => {
+  const rooms = await roomsService.getAllRooms();
+
+  res.status(200).send(rooms);


Wrap service calls in try/catch to handle unexpected errors (DB failures, etc.) and return 500 on server errors. Right now getAllRooms calls the service directly without error handling which may cause unhandled promise rejections.

mateacademy-ai-mentor · 2025-10-17T12:14:30Z

+  await roomsService.createRoom(title, userId, description);
+
+  res.sendStatus(201);


Validate the title (non-empty string) and userId (expected type/format) before calling roomsService.createRoom. Also consider returning the created room (or Location header) instead of just 201 with no body so the client can get the new room's id.

mateacademy-ai-mentor · 2025-10-17T12:14:30Z

+
+const create = async (req, res) => {
+  try {
+    const { name } = req.body;


You destructure name from the body but don't validate its type or trim whitespace. A value like ' ' will pass the current if (!name) check. Consider ensuring name is a string and doing const cleanName = typeof name === 'string' ? name.trim() : '' and validate cleanName instead.

mateacademy-ai-mentor · 2025-10-17T12:14:30Z

+    if (!name) {
+      return res.status(400).json({ message: 'Name is required' });


The presence check if (!name) is too weak for user input because whitespace-only strings are truthy. Validate after trimming (e.g., if (!cleanName) return 400). This ensures you don't create users with empty names and satisfies the requirement to validate input before DB calls.

mateacademy-ai-mentor · 2025-10-17T12:14:30Z

+      return res.status(400).json({ message: 'Name is required' });
+    }
+
+    const isNameExist = await User.findOne({ where: { name } });


You check User.findOne to return 409 when a name exists — good. However this doesn't prevent a race condition where User.create could still fail with a unique constraint error if two requests create the same name concurrently. Handle SequelizeUniqueConstraintError in the catch and return 409 to cover that case.

mateacademy-ai-mentor · 2025-10-17T12:14:30Z

+    await userService.createUser(name);
+
+    return res.status(201).json({ message: 'User was created!' });
+  } catch {


The catch block has no error parameter and returns 500 for all failures. Change to catch (err) and log the error (e.g., console.error(err)). Also detect unique constraint errors (by err.name === 'SequelizeUniqueConstraintError' or using UniqueConstraintError from Sequelize) and return 409 in that case. This maps DB errors to appropriate HTTP statuses as required.

mateacademy-ai-mentor · 2025-10-17T12:14:30Z

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


The model sets description as allowNull: false, but the model does not provide a defaultValue. The service currently uses an empty string default, but make the default explicit in the model (e.g. defaultValue: '') so direct model creations won't fail.

mateacademy-ai-mentor · 2025-10-17T12:14:30Z

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


Association uses default FK options. To guarantee every room is associated with a user (matching the controller/service which requires userId), set the foreign key to non-null and consider onDelete: 'CASCADE', e.g. Room.belongsTo(User, { foreignKey: { name: 'userId', allowNull: false }, onDelete: 'CASCADE' }) and mirror the foreignKey in User.hasMany.

mateacademy-ai-mentor · 2025-10-17T12:14:34Z

+    text: {
+      type: DataTypes.STRING,


Use DataTypes.TEXT for text rather than DataTypes.STRING. Message bodies can be longer than default STRING limits and the task expects robust text storage.

mateacademy-ai-mentor · 2025-10-17T12:14:34Z

+Message.belongsTo(User, { foreignKey: 'userId' });
+User.hasMany(Message, { foreignKey: 'userId' });
+
+Message.belongsTo(Room, { foreignKey: 'roomId' });
+Room.hasMany(Message, { foreignKey: 'roomId' });


userId and roomId columns are not explicitly defined—rely on associations alone means you can't enforce allowNull: false or add DB references/onDelete behavior. Define userId and roomId in the model (e.g. UUID type, allowNull: false, references: { model: 'users', key: 'id' }, and onDelete: 'CASCADE') so every message is guaranteed to belong to a user and a room.

mateacademy-ai-mentor · 2025-10-17T12:14:34Z

+    createdAt: {
+      type: DataTypes.DATE,
+      defaultValue: DataTypes.NOW,
+      allowNull: false,
+    },


createdAt is present and non-null with default NOW, which satisfies the requirement to expose message timestamps. Keeping updatedAt: false is fine if edits are not supported, but be explicit in commit message/docs why updates are disabled.

mateacademy-ai-mentor · 2025-10-17T12:14:34Z

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


The code rejects roomId values that are not numbers. Room IDs are UUID strings in the models, so this validation will block valid join requests. Accept non-empty strings (or validate UUID format) instead of requiring typeof roomId === 'number'.

mateacademy-ai-mentor · 2025-10-17T12:14:34Z

+      return;
+    }
+
+    const { roomId } = parsed;


You only parse JSON and check roomId, but you should validate the parsed payload structure more robustly (e.g. confirm it's an object and roomId is a non-empty string/UUID). This prevents unexpected payloads from being treated as joins.

mateacademy-ai-mentor · 2025-10-17T12:14:34Z

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


When a client joins you push the socket then fetch and send previous messages — good. Ensure messageService.getAllMessagesInRoom returns messages already mapped to { author, time, text }; otherwise you must map them before sending. Confirm the service/controller agree on a single message shape to avoid sending incomplete data.

mateacademy-ai-mentor · 2025-10-17T12:14:34Z

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


The emitter handler expects message.roomId to exist; ensure all places that emit messages (controllers/services) include roomId in the emitted payload. Without roomId the broadcast will not be routed to the appropriate room and clients won't see new messages (requirement: joined users must receive prior and live messages for the room).

mateacademy-ai-mentor · 2025-10-17T12:14:35Z

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


The code mixes shapes when formatting outgoing WS messages: it uses message.author || message.User?.name and message.time || message.createdAt. This is fragile — choose one canonical shape (prefer { author, time, text, roomId }) and broadcast that directly. That ensures every message sent to WS clients contains the required author, time, and text fields.

mateacademy-ai-mentor · 2025-10-17T12:14:35Z

@@ -0,0 +1,39 @@
+import { Message } from '../models/message.js';
+import { User } from '../models/user.model.js';


The import path is incorrect — there is no ../models/user.model.js. Import the User model from the actual module (../models/user.js) so User.findByPk/Sequelize calls don't fail at runtime.

mateacademy-ai-mentor · 2025-10-17T12:14:35Z

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


Good: getAllMessagesInRoom includes User and maps results to { author, time, text }, which satisfies the requirement that messages returned to clients include author/time/text. Ensure controllers/WS code consume this exact shape (avoid re-mapping expecting raw model instances).

mateacademy-ai-mentor · 2025-10-17T12:14:38Z

+const createMessageInRoom = async (text, userId, roomId) => {
+  const newMessage = await Message.create({ text, userId, roomId });
+
+  // Підтягуємо користувача, щоб сформувати коректний author
+  const user = await User.findByPk(userId);


createMessageInRoom creates the message and then does User.findByPk — that's fine but you can avoid the extra query by reloading the created message with include: User (or create then use Message.findByPk(newMessage.id, { include: User })). This centralizes mapping in the service and avoids redundant DB calls.

mateacademy-ai-mentor · 2025-10-17T12:14:38Z

+  return {
+    author: user?.name || 'Unknown',
+    time: newMessage.createdAt,
+    text: newMessage.text,
+  };


The returned object from createMessageInRoom lacks roomId. Emitting/broadcasting logic elsewhere (WS emitter/controller) expects roomId to route messages to the correct room. Include roomId in the returned/mapped object (e.g. { author, time, text, roomId }) so emitted messages can be delivered to joined clients.

mateacademy-ai-mentor · 2025-10-17T12:14:38Z

+const getAllRooms = () => {
+  return Room.findAll();


getAllRooms is fine as-is, but consider adding ordering (e.g. by createdAt) or including related data if controllers need it. Not required, but can improve client UX when listing rooms.

mateacademy-ai-mentor · 2025-10-17T12:14:39Z

+const createRoom = (title, userId, description = '') => {
+  return Room.create({ title, userId, description });


createRoom does not validate title and userId before calling the DB. Consider validating inputs here and throwing a clear error (or returning a rejected promise) so controllers can return a 400 response instead of relying on DB errors.

mateacademy-ai-mentor · 2025-10-17T12:14:39Z

+const deleteRoom = (id) => {
+  return Room.destroy({ where: { id } });


deleteRoom silently calls Room.destroy with the provided id. It's safer to validate id and/or return the destroyed count so callers can translate a missing room into a 404 response if nothing was deleted.

mateacademy-ai-mentor · 2025-10-17T12:14:39Z

+const updateRoom = (id, title, description) => {
+  if (!id) {
+    throw new Error('Room id is required');
+  }
+
+  const updatedData = {};
+
+  if (title) {
+    updatedData.title = title;
+  }
+
+  if (description) {
+    updatedData.description = description;
+  }
+
+  if (Object.keys(updatedData).length === 0) {
+    throw new Error('At least one field must be provided');
+  }
+
+  return Room.update(updatedData, { where: { id } });


updateRoom currently throws when id is missing or when no fields are provided (good). However, Room.update returns an array like [affectedCount]; consider checking that count and returning or throwing when no rows were updated so controllers can return 404 for non-existent rooms.

mateacademy-ai-mentor

Review Summary

Here is my approval to help you keep momentum while learning. Overall, you set up a solid server with routes, models, services, and WebSocket handling. Nice work normalizing messages in the service and implementing room CRUD.

Critical fixes to address next:

src/controllers/message.controller.js: Fix the import to ../models/user.js and align with the service’s return shape (author, time, text, roomId); remove the extra User.findByPk and include roomId in emitted/returned payloads.
src/index.js: Allow UUID string roomIds in the WS join check and include roomId when broadcasting; also wrap history fetch in try/catch.
src/controllers/rooms.controller.js: Use 400 (not 404) for missing fields, trim/validate input, and wrap service calls in try/catch to map not-found to 404 and unexpected errors to 500.
src/controllers/user.controller.js: Trim name and handle SequelizeUniqueConstraintError in catch (err) to return 409 consistently.

Minor improvements:

src/models/message.js and src/models/room.js: Add explicit non-null UUID FKs (userId, roomId) and consider DataTypes.TEXT for message text; give description a safe default.
services: Prefer returning created/updated instances and using specific error types to help controllers respond with precise statuses.

Your solution is approved; great progress so far—addressing these items will make your chat app robust and production-friendly.

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Rate AI review example ↩

mateacademy-ai-mentor · 2025-10-17T12:47:11Z

+
+const create = async (req, res) => {
+  try {
+    const { name } = req.body;


Trim incoming name and validate the trimmed value. Currently if (!name) will allow strings containing only whitespace (e.g. " ") — trim and check if (!name || !name.trim()) so empty/whitespace-only names are rejected.

mateacademy-ai-mentor · 2025-10-17T12:47:11Z

+      return res.status(409).json({ message: 'User already exists' });
+    }
+
+    await userService.createUser(name);


You're proactively checking for an existing user which is good, but there's still a race condition between this check and createUser. Consider handling the unique constraint error from the DB during creation instead of relying only on the pre-check; that makes the endpoint safe for concurrent requests.

mateacademy-ai-mentor · 2025-10-17T12:47:11Z

+    await userService.createUser(name);
+
+    return res.status(201).json({ message: 'User was created!' });
+  } catch {


The catch block doesn't capture the thrown error object. Use catch (err) and handle SequelizeUniqueConstraintError (return 409) or at least log err before returning 500 so one can diagnose failures. This also lets you return a meaningful status when DB uniqueness is violated.

mateacademy-ai-mentor · 2025-10-17T12:47:11Z

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


Returning 404 for missing title/userId is incorrect — this is a client validation failure. Use 400 (Bad Request), return a helpful JSON message, and consider trimming title before validating.

mateacademy-ai-mentor · 2025-10-17T12:47:12Z

+  await roomsService.createRoom(title, userId, description);
+
+  res.sendStatus(201);


Call to roomsService.createRoom is not guarded. Wrap in try/catch to handle service errors (e.g. unique constraint or DB errors). Also consider returning the created room object (201 with body) rather than an empty 201 to help clients.

mateacademy-ai-mentor · 2025-10-17T12:47:12Z

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


Validation here uses 404 for missing title/description or roomId. Missing request fields should be 400. Also validate/trim title/description and ensure roomId is a non-empty UUID string (or return 400).

mateacademy-ai-mentor · 2025-10-17T12:47:12Z

+  await roomsService.updateRoom(roomId, title, description);
+
+  res.sendStatus(204);


roomsService.updateRoom may throw 'Room not found' — currently the controller always responds 204. Wrap the service call in try/catch and return 404 when the service indicates the room doesn't exist, otherwise 500 for unexpected errors.

mateacademy-ai-mentor · 2025-10-17T12:47:12Z

+  await roomsService.deleteRoom(roomId);
+
+  res.sendStatus(204);


In deleteRoom, you validate roomId (good) but don't handle service errors. roomsService.deleteRoom throws when the room doesn't exist — wrap the call in try/catch and return 404 for not-found, 500 for other errors. Keeping the 400 for missing roomId is appropriate.

mateacademy-ai-mentor · 2025-10-17T12:47:12Z

+  const rooms = await roomsService.getAllRooms();
+
+  res.status(200).send(rooms);


getAllRooms has no error handling. Consider surrounding the service call with try/catch and return 500 on unexpected errors to avoid unhandled rejections impacting the server process.

mateacademy-ai-mentor · 2025-10-17T12:47: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 incorrect — there is no ../models/user.model.js. The User model is defined at ../models/user.js; update this import to avoid a module-not-found error at runtime.

mateacademy-ai-mentor · 2025-10-17T12:47:18Z

+  try {
+    const newMessage = await messageService.createMessageInRoom(
+      text,
+      userId,
+      roomId,
+    );
+
+    const user = await User.findByPk(userId);
+
+    if (!user) {
+      return res.status(404).json({ message: 'User not found' });
+    }


You call messageService.createMessageInRoom(...) then immediately do a User.findByPk(userId) and later use newMessage.createdAt. The service already returns a normalized object (author, time, text, roomId). This mismatch will produce undefined fields and an unnecessary DB lookup. Either have the service return a Sequelize instance including User or update the controller to use newMessage.author / newMessage.time / newMessage.text and remove the extra User.findByPk call.

mateacademy-ai-mentor · 2025-10-17T12:47:18Z

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


formattedMessage uses user.name and newMessage.createdAt/newMessage.text but the service returns author/time/text. Also the returned/emitted object is missing roomId — the WS broadcaster expects roomId to route messages to rooms. Include roomId and use the service's fields (or change the service to return matching names).

mateacademy-ai-mentor · 2025-10-17T12:47:18Z

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


You emit formattedMessage without roomId. The listener in src/index.js reads message.roomId to decide the target room; without it messages will not be delivered to clients in the correct room. Add roomId to the emitted payload.

mateacademy-ai-mentor · 2025-10-17T12:47:19Z

+
+    messageEmitter.emit('message', formattedMessage);
+
+    res.status(201).json(formattedMessage);


The response res.status(201).json(formattedMessage) returns a payload without roomId. The client expects messages to include roomId for correct handling; include roomId in the response as well.

mateacademy-ai-mentor · 2025-10-17T12:47:19Z

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


In getMessages you map messages expecting Sequelize fields (m.User?.name, m.createdAt, m.text), but messageService.getAllMessagesInRoom returns normalized objects (author, time, text, roomId). Use m.author and m.time (and include roomId if needed), or change the service to return Sequelize instances with User included so the shapes match.

mateacademy-ai-mentor · 2025-10-17T12:47:19Z

+    text: {
+      type: DataTypes.STRING,


Use a longer text type for message bodies. DataTypes.STRING imposes length limits; consider DataTypes.TEXT so long messages won't be truncated or rejected.

mateacademy-ai-mentor · 2025-10-17T12:47:19Z

+    createdAt: {
+      type: DataTypes.DATE,
+      defaultValue: DataTypes.NOW,
+      allowNull: false,
+    },
+  },


Define userId and roomId explicitly in the model attributes (e.g. type: DataTypes.UUID, allowNull: false, references: { model: 'users', key: 'id' }). Right now the associations (below) will create these columns implicitly but without explicit NOT NULL constraints — the task recommends non-null FKs so messages always belong to a user and a room.

mateacademy-ai-mentor · 2025-10-17T12:47:19Z

+Message.belongsTo(User, { foreignKey: 'userId' });
+User.hasMany(Message, { foreignKey: 'userId' });
+
+Message.belongsTo(Room, { foreignKey: 'roomId' });
+Room.hasMany(Message, { foreignKey: 'roomId' });


Associations are correct, but they rely on implicitly-created FK columns. After adding explicit userId/roomId attributes, keep these belongsTo/hasMany lines so Sequelize sets up relations and foreign keys consistently.

mateacademy-ai-mentor · 2025-10-17T12:47:19Z

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


description is declared allowNull: false but has no defaultValue. If any code path creates a Room without providing description the DB will error. Either set defaultValue: '' here or ensure every create call always supplies a non-null description (service currently defaults, but making the model resilient is safer).

mateacademy-ai-mentor · 2025-10-17T12:47:19Z

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


Associations are declared but there is no explicit userId attribute on the Room model. The service createRoom(title, userId, ...) expects a userId — add a userId field (UUID, allowNull: false) to the model and declare the association with foreignKey: 'userId' so the DB enforces the relationship.

mateacademy-ai-mentor · 2025-10-17T12:47:23Z

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


When adding the explicit userId field, ensure the association uses the same foreign key, e.g. Room.belongsTo(User, { foreignKey: 'userId' }) and User.hasMany(Room, { foreignKey: 'userId' }), to keep associations and constraints consistent across models and services.

mateacademy-ai-mentor · 2025-10-17T12:47:24Z

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


The WS join validation requires roomId to be a number. Room IDs are UUID strings in this project — this check will prevent clients from joining. Accept non-empty strings (or validate UUID format) instead of typeof roomId !== 'number' so join requests succeed.

mateacademy-ai-mentor · 2025-10-17T12:47:24Z

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


messageService.getAllMessagesInRoom(roomId) is called without error handling. If the service throws (invalid id, DB error) this will produce an unhandled rejection. Wrap the call in try/catch and return/log an appropriate error instead of letting the handler fail silently.

mateacademy-ai-mentor · 2025-10-17T12:47:24Z

+  const { roomId } = message;
+
+  if (!roomId || !rooms[roomId]) {
+    return;


This handler expects incoming emitted message objects to include roomId. If upstream emitters/controllers don't include roomId, this check (if (!roomId || !rooms[roomId])) will silently skip broadcasting. Ensure all emitters include roomId when emitting events.

mateacademy-ai-mentor · 2025-10-17T12:47:24Z

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


The formatted object sent to clients omits roomId and mixes field access patterns (message.author vs message.User?.name, message.time vs message.createdAt). Unify the message shape across the app (prefer a normalized object: author, time, text, roomId) and include roomId when sending to clients so payloads are consistent and the client can rely on one schema.

mateacademy-ai-mentor · 2025-10-17T12:47:24Z

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


Accessing message.User?.name and message.createdAt assumes a Sequelize instance; the service currently returns normalized fields (author, time). Update these lines to use message.author and message.time (or change the service to return instances) to avoid undefined values at runtime.

mateacademy-ai-mentor · 2025-10-17T12:47:24Z

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


When including User via Sequelize, the included model is typically available on the User property (capitalized), not user. msg.user?.name will be undefined; use msg.User?.name (or explicitly alias the include) so author is populated correctly.

mateacademy-ai-mentor · 2025-10-17T12:47:24Z

+  const messageWithUser = await Message.findByPk(newMessage.id, {
+    include: [{ model: User, attributes: ['name'] }],
+  });


After Message.findByPk(...) you assume messageWithUser exists. Add a null check and throw or handle the error if it's not found to avoid runtime exceptions when accessing properties on null.

mateacademy-ai-mentor · 2025-10-17T12:47:24Z

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


Same issue as above: messageWithUser.user?.name should reference the included model property messageWithUser.User?.name (capital U) or use an explicit alias in the include. Otherwise author will be 'Unknown' even when the user exists.

mateacademy-ai-mentor · 2025-10-17T12:47:24Z

+  return {
+    author: messageWithUser.user?.name || 'Unknown',
+    time: messageWithUser.createdAt,
+    text: messageWithUser.text,
+    roomId: messageWithUser.roomId,
+  };


This service returns a normalized plain object ({ author, time, text, roomId }). The controller currently expects Sequelize instances (accessing .createdAt and .User). Align the contract: either return the full instance including User from the service, or update the controller to use the normalized fields (time, author, etc.). Without alignment, message fields will be undefined when controllers/emitters try to access them.

mateacademy-ai-mentor · 2025-10-17T12:47:30Z

+roomsRoute.get('/', roomController.getAllRooms);
+roomsRoute.post('/', roomController.createRoom);


The file registers GET, POST, PATCH and DELETE routes which cover list, create, rename/update and delete room operations. However there is no explicit HTTP "join room" endpoint here. If joining is intentionally handled via WebSocket (src/index.js), please document that for clients or add an HTTP join handler for clients that expect to join via REST.

mateacademy-ai-mentor · 2025-10-17T12:47:30Z

+roomsRoute.patch('/:roomId', roomController.updateRoom);
+roomsRoute.delete('/:roomId', roomController.deleteRoom);


Consider adding validation for :roomId (e.g. check non-empty / UUID format) as middleware on the PATCH and DELETE routes so malformed IDs are rejected early with a 400. You can use router.param or a small middleware before the controller call.

mateacademy-ai-mentor · 2025-10-17T12:47:31Z

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


Validation for title and userId is minimal. Consider trimming title and description and validating that title isn’t an empty string after trimming. Also, rather than throwing a generic Error here, consider throwing a specific error (or a custom error type) so the controller can return a 400 status with a helpful message to the client.

mateacademy-ai-mentor · 2025-10-17T12:47:31Z

+    throw new Error('Title and userId are required');
+  }
+
+  return Room.create({ title, userId, description });


Room.create returns the created instance — good — but controllers currently only return status codes. Consider returning the created room object here (or ensure the controller returns it) so clients can receive the new room data (id, createdAt, etc.). Also consider handling SequelizeUniqueConstraintError here if title must be unique.

mateacademy-ai-mentor · 2025-10-17T12:47:31Z

+  if (deletedCount === 0) {
+    throw new Error('Room not found');
+  }
+
+  return deletedCount;


When deleting, you throw 'Room not found' if no rows were deleted which is correct, but consider using a specific error type so controllers can map it to 404. Also think about related data: if messages belong to a room, ensure database FK constraints or cascade delete behavior are defined so deleting a room doesn't leave orphan messages or silently fail elsewhere.

mateacademy-ai-mentor · 2025-10-17T12:47:31Z

+const updateRoom = async (id, title, description) => {
+  if (!id) {
+    throw new Error('Room id is required');
+  }
+
+  const updatedData = {};
+
+  if (title) {
+    updatedData.title = title;
+  }
+
+  if (description) {
+    updatedData.description = description;
+  }
+
+  if (Object.keys(updatedData).length === 0) {
+    throw new Error('At least one field must be provided');
+  }
+
+  const [updatedCount] = await Room.update(updatedData, { where: { id } });
+
+  if (updatedCount === 0) {
+    throw new Error('Room not found');
+  }
+
+  return updatedCount;


updateRoom currently returns the updated row count. For better API ergonomics, consider returning the updated room object instead (use Room.update(updatedData, { where: { id }, returning: true }) on Postgres) or fetch the room after update. Also validate title/description (trim) and verify id is a valid UUID before calling the DB to avoid unnecessary queries.

mateacademy-ai-mentor · 2025-10-17T12:47:31Z

+const getAllRooms = async () => {
+  return Room.findAll({
+    order: [['createdAt', 'DESC']],
+  });


getAllRooms returns all rooms without pagination or error handling. At minimum, wrap calls in try/catch in controllers (or let the service throw and controllers map errors). If the dataset can grow, consider adding pagination parameters to avoid heavy responses.

mateacademy-ai-mentor · 2025-10-17T12:47:32Z

+const createUser = async (name) => {
+  return User.create({ name });


Validate and normalize input before creating the user. For example, trim name and check it's not an empty string to avoid creating users with only whitespace.

mateacademy-ai-mentor · 2025-10-17T12:47:32Z

+const createUser = async (name) => {
+  return User.create({ name });
+};


Consider catching SequelizeUniqueConstraintError here (or let the controller handle it) and rethrowing a meaningful error so the HTTP layer can return 409 Conflict. As-is, DB constraint errors will bubble up as generic 500s unless handled elsewhere.

Solution

fadb9c1

mateacademy-ai-mentor suggested changes Oct 17, 2025

View reviewed changes

mateacademy-ai-mentor reviewed Oct 17, 2025

View reviewed changes

Solution

baee464

DSemeniukV requested a review from mateacademy-ai-mentor October 17, 2025 12:08

mateacademy-ai-mentor suggested changes Oct 17, 2025

View reviewed changes

mateacademy-ai-mentor reviewed Oct 17, 2025

View reviewed changes

Solution

013757c

DSemeniukV requested a review from mateacademy-ai-mentor October 17, 2025 12:40

mateacademy-ai-mentor approved these changes Oct 17, 2025

View reviewed changes

mateacademy-ai-mentor reviewed Oct 17, 2025

View reviewed changes

		if (!roomId \|\| !userId \|\| !text) {
		return res.sendStatus(404);


		messageEmitter.emit('message', newMessage);

		res.status(201).json(newMessage);


		rooms[roomId].push(ws);

		const messages = await Message.findAll({ where: { roomId } });

		@@ -0,0 +1,14 @@
		import { Message } from '../models/message.js';

		const getAllMessagesInRoom = (roomId) => {
		return Message.findAll({ where: { roomId } });

		const createMessageInRoom = (text, userId, roomId) => {
		return Message.create({ text, userId, roomId });

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

		await roomsService.createRoom(title, userId, description);

		res.sendStatus(201);

		if (!name) {
		return res.status(400).json({ message: 'Name is required' });


		const { roomId } = parsed;

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

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

		@@ -0,0 +1,39 @@
		import { Message } from '../models/message.js';
		import { User } from '../models/user.model.js';

		const createRoom = (title, userId, description = '') => {
		return Room.create({ title, userId, description });

		const deleteRoom = (id) => {
		return Room.destroy({ where: { id } });

		await roomsService.updateRoom(roomId, title, description);

		res.sendStatus(204);

		const rooms = await roomsService.getAllRooms();

		res.status(200).send(rooms);


		messageEmitter.emit('message', formattedMessage);

		res.status(201).json(formattedMessage);

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

		roomsRoute.get('/', roomController.getAllRooms);
		roomsRoute.post('/', roomController.createRoom);

		roomsRoute.patch('/:roomId', roomController.updateRoom);
		roomsRoute.delete('/:roomId', roomController.deleteRoom);

		const createUser = async (name) => {
		return User.create({ name });

Conversation

DSemeniukV commented Oct 17, 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

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