feat: implement chat using websocket by mykhailokurochkin · Pull Request #132 · mate-academy/node_chat

mykhailokurochkin · 2025-09-22T10:12:22Z

No description provided.

mateacademy-ai-mentor

Review Summary

Great job setting up the server foundation with Express, Sequelize, and WebSockets! The project structure is clean and the database connection is solid. However, I'm requesting changes because several core features from the task description are missing.

To move forward, please focus on these key areas:

Room Management: The application currently works as a single global chat. It needs the core functionality to create, join, and manage separate chat rooms. This will require a new data model for rooms and updates to your routes, services, and WebSocket logic to handle room-specific messages.
Incomplete Message Data: The task requires every message to have an author, time, and text. Your Message model in src/models/Message.js is missing the author and time fields. This needs to be updated throughout the application, from the model to the service and controller layers.
Message History: When a user joins a room, they should receive the existing message history. This feature is currently missing from the WebSocket connection logic in src/index.js.

✨ 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-09-22T10:14:30Z

@@ -0,0 +1,69 @@
+# React + TypeScript + Vite


This appears to be the default README file from the Vite template. It's good practice to update this file to describe your own project. Consider including:

A brief description of your chat application.

Instructions on how to install dependencies and run the project.

Information about any required environment variables.

mateacademy-ai-mentor · 2025-09-22T10:14:30Z

+const getAll = async (req, res) => {
+  const messages = await messageService.getAll();
+
+  res.send(messages);
+};


This function is designed to fetch all messages, but the task requires a room-based chat system. For that to work, you would need a way to fetch messages that belong only to a specific room. Consider how you might change this function to accept a room identifier and return only the messages for that room.

mateacademy-ai-mentor · 2025-09-22T10:14:30Z

+const add = async (req, res) => {
+  try {
+    const messageData = req.body;
+    const newMessage = await messageService.add(messageData);
+    res.status(201).send(newMessage);
+  } catch (error) {
+    res.status(500).send({ error: error.message });
+  }
+};


According to the task description, all messages must have an author, time, and text. The current implementation in the messageService and Message model only seems to handle the text property.

How could you modify this add function and the service layer to ensure that the author and time are also saved with each new message?

mateacademy-ai-mentor · 2025-09-22T10:14:30Z

+  });
+}
+
+wss.on('connection', (connection) => {


According to the requirements, 'New user should see all prev messages in the room'. When a client establishes a connection, the server should send them the existing message history for the room they are joining. Currently, no messages are sent upon connection.

mateacademy-ai-mentor · 2025-09-22T10:14:30Z

+  connection.on('message', async (messageBuffer) => {
+    try {
+      const messageData = JSON.parse(messageBuffer.toString());
+      const newMessage = await messageService.add(messageData);


The task requires messages to include an author and time in addition to the text. The messageService.add function and the corresponding Message model seem to only handle the text field. This needs to be updated to store and broadcast all the required message information.

mateacademy-ai-mentor · 2025-09-22T10:14:31Z

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


The task description specifies that 'All the messages should have an author, time and text'. This model is currently missing the author and time fields.

Additionally, to support the rooms feature, each message will need to belong to a specific room. Think about what kind of field you would need to add here to create that association (for example, a roomId).

mateacademy-ai-mentor · 2025-09-22T10:14:31Z

+
+const messageRouter = Router();
+
+messageRouter.get('/messages', messageController.getAll);


This route fetches all messages. According to the task requirements, the application must support multiple chat rooms, and users joining a room should see its specific message history. How would a client fetch messages for only one room with this endpoint? Consider making your routes room-specific, for instance: GET /rooms/:roomId/messages.

mateacademy-ai-mentor · 2025-09-22T10:14:31Z

+const messageRouter = Router();
+
+messageRouter.get('/messages', messageController.getAll);
+messageRouter.post('/messages', messageController.add);


Similar to the GET route, this POST endpoint adds a message globally. To fulfill the requirements, a message must belong to a specific room. This route should be updated to reflect that, for example, by changing the path to /rooms/:roomId/messages and ensuring the message is associated with the correct room when it's created.

mateacademy-ai-mentor · 2025-09-22T10:14:31Z

+async function getAll() {
+  const result = await Message.findAll();
+  return result;
+}


The task requires a chat application with rooms. This function fetches all messages globally. For the application to work as expected, you'll need a way to retrieve messages that belong to a specific room. How could you modify this function to accept a room ID and return only the messages for that room?

mateacademy-ai-mentor · 2025-09-22T10:14:31Z

+const add = async ({ text }) => {
+  return await Message.create({ text });
+};


According to the task description, every message must have an author, time, and text. This function is only saving the text.

You'll need to update this function to accept and save all three required fields. This will also require a change in the Message model to include columns for author and time.

mateacademy-ai-mentor

Review Summary

Decision: Requesting changes because there are critical blockers that prevent core requirements from functioning correctly.

Critical issues to fix (blockers):

WebSocket room scoping (src/index.js): messages are broadcast to all clients regardless of room. This breaks room isolation and live updates per room (Reqs 1.5, 1.6, 1.7, 3.4). Implement a WS join flow (e.g., handle { type: 'join', roomId }), track connection→room, and broadcast only to subscribers of that room. Also send history on join or ensure the client fetches history and the WS is room-scoped.
Missing join confirmation/handling: there’s no explicit server-side join confirmation (REST or WS). Add a WS join handler or a POST /rooms/:id/join endpoint that validates the room and returns success/error (Req 2.6, join requirement).
Username not sent to server (client/Username.tsx and/or client/App.tsx): the client does not notify the server when a username is set (Reqs 1.2, 2.2). Send a REST/WS event (e.g., { type: 'set-username', username }) when saving.
message.controller delete response (src/controllers/message.controller.js): in deleteByRoom, you set status but never send/end the response. Replace with res.sendStatus(200) or res.status(200).send(...) and likewise for the 404 branch (Req 2.6).
room.controller rename response (src/controllers/room.controller.js): rename returns 201 with null when not found. Check the result and return 404 on failure; 200/201 with the updated room on success (Req 2.6).
Client rename state mutation (client/App.tsx): rooms.find(...).name = ... mutates state in place, preventing React re-render. Update state immutably with setRooms(prev => prev.map(...)) so rename is reflected (Req 3.3).

Important consistency fixes (not blockers but recommended):

Client WS handling: send a join message after socket open and defensively filter incoming messages by message.roomId === currentRoom.
Message time type: align Message.time typing with actual JSON (string) or convert to Date consistently (Reqs 3.2, 3.5).
Service robustness (src/services/message.service.js): validate payloads in add, return plain JSON via toJSON(), and set explicit order in getByRoom.
CSS/UI: add .message, .author, .time, .text styles in client/App.css to clearly render required fields; consider switching .messages to flex-direction: column and use overflow: auto.

You’ve built a solid foundation (models, REST routes, client UI), but the WS room-scoping and response handling must be fixed to meet the core requirements. Once these blockers are addressed, the app should satisfy the checklist reliably. Keep going — you’re close.

✨ 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-09-24T13:42:48Z

@@ -0,0 +1,34 @@
+.messages {


The task requires the client to render message objects containing author, time, and text (checklist items 1.4 and 3.6). This stylesheet defines the container .messages but does not include any styles for individual message elements such as .message, .author, .time, or .text. Add these classes so the UI clearly presents the required fields and so tests / reviewers can verify the rendered structure.

mateacademy-ai-mentor · 2025-09-24T13:42:48Z

@@ -0,0 +1,34 @@
+.messages {
+  display: flex;
+  flex-direction: column-reverse;


Using flex-direction: column-reverse will reverse visual order (new messages appear at the top). This can complicate keyboard navigation, screen readers, testing, and JS auto-scrolling. Consider: (a) using normal flex-direction: column and appending messages to the end of the container, then scroll to bottom in JS, or (b) keep DOM order reversed intentionally but document why and ensure screen-reader ordering is handled. If you keep column-reverse, you must ensure message history ordering and joins work as expected in the client logic (task requires room-specific history delivery).

mateacademy-ai-mentor · 2025-09-24T13:42:48Z

+  min-height: 200px;
+  max-height: 200px;


Using both min-height: 200px and max-height: 200px is redundant when you want a fixed size. Use height: 200px to make intent explicit. Also consider a CSS variable or relative unit if you need responsiveness.

mateacademy-ai-mentor · 2025-09-24T13:42:49Z

+  flex-direction: column-reverse;
+  min-height: 200px;
+  max-height: 200px;
+  overflow: scroll;


overflow: scroll forces scrollbars to always show. Prefer overflow: auto so scrollbars only appear when needed. For mobile smooth scrolling add -webkit-overflow-scrolling: touch; if appropriate.

mateacademy-ai-mentor · 2025-09-24T13:42:49Z

+.input {
+  display: flex;
+  gap: 24px;
+}
+
+.inputs {


There are two similar class names: .input and .inputs. This is likely to cause confusion (are both used? which is the canonical one?). Consider consolidating to a single clear name like .input-group or .controls and update the HTML accordingly to avoid accidental style omissions.

mateacademy-ai-mentor · 2025-09-24T13:42:53Z

+const deleteRoom = async (id) => {
+  const deletedCount = await Room.destroy({
+    where: { id },
+  });
+  return deletedCount > 0;


deleteRoom destroys the Room row, but consider coordinating message cleanup to avoid orphaned messages. Either rely on DB-level ON DELETE CASCADE if configured, or perform message deletions in the same service method within a transaction so room + messages are removed atomically. I noticed message deletion is performed elsewhere — centralizing it here (or ensuring transactionality) improves consistency.

mateacademy-ai-mentor · 2025-09-24T13:42:53Z

+  } catch (error) {
+    throw new Error('Failed to add room: ' + error.message);


Consistency: add wraps Sequelize errors into a new Error with a custom message. For consistent error handling, either follow the same pattern in other service functions or standardize to throwing specific error types so controllers can map them to HTTP statuses uniformly.

mateacademy-ai-mentor · 2025-09-24T13:42:54Z

+export const client = new Sequelize({
+  host: process.env.DB_HOST,
+  username: process.env.DB_USER,
+  password: process.env.DB_PASSWORD,
+  database: process.env.DB_DATABASE,
+  dialect: 'postgres',


You construct Sequelize directly from environment variables without validating them. If any of DB_HOST, DB_USER, DB_PASSWORD, or DB_DATABASE are missing the connection will fail with a low-level error during startup. Consider supporting process.env.DATABASE_URL as a fallback or add explicit checks and a helpful error message so failures are easier to diagnose.

mateacademy-ai-mentor · 2025-09-24T13:42:54Z

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


Consider adding common Sequelize options to improve robustness. For example:

logging: false to silence SQL logs in dev/CI or provide a logger

port if your DB uses a non-default port

pool settings (max/min/idle) and dialectOptions (e.g., SSL) for production DBs
These are not required but help avoid noisy logs and connection issues in various environments.

mateacademy-ai-mentor · 2025-09-24T13:42:54Z

+import 'dotenv/config';
+import { Sequelize } from 'sequelize';


Importing dotenv/config at the top is fine to load environment variables during startup. Ensure your process loads .env before other modules if you rely on those vars in other modules executed at import time. No change required here unless you observe ordering issues.

mateacademy-ai-mentor

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

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.

… isolation on server side

mateacademy-ai-mentor

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

Review Summary

Educational approval: great progress implementing the core flows (rooms CRUD, join via WS, history delivery, username persistence/sending). The UI and server are largely in place and the code is readable and well-structured.

Critical improvements to focus on next:

WebSocket lifecycle and room scoping (App.tsx): create the socket once, send stored username on open, and filter incoming messages by roomId === currentRoom (see lines 153–163). Avoid duplicating history delivery (either WS on join or REST fetch, not both; lines 119–147, 150–178).
Message integrity and timestamps (server): validate author, text, and roomId in controllers/services, and set time server-side (ignore client time) to meet consistent-time requirement (message.controller.js lines 15–19; message.service.js add()).
Consistent ordering and serialization: make DB queries return messages in a defined order (no ad-hoc reverse()), and send plain JSON (use .toJSON()) over REST/WS to ensure predictable payloads (index.js history/broadcast; services/controllers returns).

Minor but helpful: in CSS switch overflow: scroll to overflow: auto and add .message/.author/.time styles; in Username.tsx trim input and check socket.readyState === WebSocket.OPEN before sending; align client fetches to the Vite proxy (/api/...); fix the Message.time type mismatch (use string or convert on receive).

You’re very close—addressing these will solidify room isolation, stable history, and consistent timestamps. Nicely done so far—keep it up!

✨ 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-09-24T15:16:51Z

@@ -0,0 +1,34 @@
+.messages {
+  display: flex;
+  flex-direction: column-reverse;


Using flex-direction: column-reverse here will reverse visual order of messages and interacts poorly with ad-hoc reverse() calls or prepending messages in JS. Pick one canonical ordering strategy (either keep column and append messages, or keep column-reverse and ensure all code and server history use that convention). See checklist: messages must be correctly ordered and history must be shown on join.

mateacademy-ai-mentor · 2025-09-24T15:16:51Z

+  flex-direction: column-reverse;
+  min-height: 200px;
+  max-height: 200px;
+  overflow: scroll;


Prefer overflow: auto over overflow: scroll so a scrollbar only appears when necessary. This improves UX and avoids an always-visible disabled scrollbar.

mateacademy-ai-mentor · 2025-09-24T15:16:51Z

+.messages {
+  display: flex;
+  flex-direction: column-reverse;
+  min-height: 200px;
+  max-height: 200px;
+  overflow: scroll;
+}


There are no styles for individual message fields like .message, .author, .time, or .text. The task requires showing author and time for every message — add dedicated selectors so those fields are legible and consistent across messages.

mateacademy-ai-mentor · 2025-09-24T15:16:51Z

+.messages {
+  display: flex;
+  flex-direction: column-reverse;
+  min-height: 200px;


Consider making the message container a normal column flow and using justify-content: flex-end or client-side scroll handling to show newest messages at the bottom. This avoids fragile reverse() logic in JS and makes scrolling predictable.

mateacademy-ai-mentor · 2025-09-24T15:16:51Z

+  flex-direction: column;
+
+  gap: 8px;


Small readability note: .roomsList block has an extra blank line between flex-direction and gap (lines 26–28). It's cosmetic only; no functional change required.

mateacademy-ai-mentor · 2025-09-24T15:16:57Z

+export const roomService = {
+  getAll,
+  add,
+  deleteRoom,
+  rename,
+};


General: service methods return Sequelize instances. For API safety and predictable JSON payloads, convert instances to plain objects before returning (use .toJSON()), or have controllers do so. This avoids sending instance metadata over the network and keeps the API contract clear.

mateacademy-ai-mentor · 2025-09-24T15:16:57Z

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


Importing dotenv/config at the top ensures environment variables are loaded — that's fine. Consider validating required env vars after this import so you fail fast with a clear error if e.g. DB_HOST/DB_USER/DB_DATABASE are missing.

mateacademy-ai-mentor · 2025-09-24T15:16:57Z

+  host: process.env.DB_HOST,
+  username: process.env.DB_USER,
+  password: process.env.DB_PASSWORD,
+  database: process.env.DB_DATABASE,


The Sequelize constructor uses process.env values directly. To make startup errors clearer and avoid subtle runtime issues, validate and/or provide sensible defaults for these env vars (or throw an error if critical vars are missing). This will make debugging misconfigured environments easier.

mateacademy-ai-mentor · 2025-09-24T15:16:57Z

+export const client = new Sequelize({
+  host: process.env.DB_HOST,
+  username: process.env.DB_USER,
+  password: process.env.DB_PASSWORD,
+  database: process.env.DB_DATABASE,
+  dialect: 'postgres',
+});


Consider adding logging: false (or conditional logging based on NODE_ENV), pool settings, and potentially timezone or define defaults to the Sequelize options for predictable behavior across environments.

mateacademy-ai-mentor · 2025-09-24T15:16:57Z

+  dialect: 'postgres',
+});


If you deploy to environments that require SSL (e.g., managed Postgres providers), add dialectOptions: { ssl: { require: true, rejectUnauthorized: false } } to the constructor. Without this, connections to some hosted Postgres servers may fail.

mateacademy-ai-mentor

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.

feat: implement chat using websocket

9a861ed

mateacademy-ai-mentor suggested changes Sep 22, 2025

View reviewed changes

feat: implement rooms, ability to create, rename, join and delete them

6861111

mykhailokurochkin requested a review from mateacademy-ai-mentor September 24, 2025 13:05

fix: fix potential issues

4b2ef46

mateacademy-ai-mentor suggested changes Sep 24, 2025

View reviewed changes

mateacademy-ai-mentor reviewed Sep 24, 2025

View reviewed changes

feat: add username sending to server, room join confirmation and room…

791b55a

… isolation on server side

mykhailokurochkin requested a review from mateacademy-ai-mentor September 24, 2025 14:21

mykhailokurochkin added 2 commits September 24, 2025 17:39

fix: add joinRoom function

87f7fe2

fix: fix potential issue

fab06bb

mateacademy-ai-mentor reviewed Sep 24, 2025

View reviewed changes

mateacademy-ai-mentor approved these changes Sep 24, 2025

View reviewed changes

mateacademy-ai-mentor reviewed Sep 24, 2025

View reviewed changes


		const messageRouter = Router();

		messageRouter.get('/messages', messageController.getAll);

		} catch (error) {
		throw new Error('Failed to add room: ' + error.message);

		import 'dotenv/config';
		import { Sequelize } from 'sequelize';

Conversation

mykhailokurochkin commented Sep 22, 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!

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!

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!

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