Solution by alyonashunevych · Pull Request #192 · mate-academy/node_chat

alyonashunevych · 2026-03-30T13:02:46Z

No description provided.

mateacademy-ai-mentor

Review Summary

I am approving your project! You've built an excellent full-stack chat application that meets all the core requirements. The architecture is solid, with a great separation of concerns between the client and server, and the real-time features are implemented very well.

While your solution is approved, I have a couple of suggestions to make your code even more robust:

React List Keys: In MessageList.tsx and RoomsList.tsx, you're using the array index as the key. It's a best practice to use a stable, unique identifier from your data, like message.id or room.id, to prevent potential rendering bugs.
Form Submission State: In LoginPage.tsx, the setSubmitting(false) call runs immediately. To provide accurate UI feedback, this should be moved into a .finally() block on the promise chain to ensure it executes after the API call is complete.

Great work on this complex project!

✨ 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 · 2026-03-30T13:10:53Z

src/client/components/Room/MessageList/MessageList.tsx

+          <div className={styles.message_list}>
+            {messages.map((message, index) => (
+              <div
+                key={index}


Using the array index as a key is an anti-pattern in React. While it might seem to work here because messages are only added to the end, it can lead to unpredictable bugs if the list were to be re-ordered or if items were removed from the middle. It's best practice to use a stable and unique identifier from the data itself. In this case, message.id would be a perfect key.

mateacademy-ai-mentor · 2026-03-30T13:10:53Z

src/client/components/Rooms/RoomsList/RoomsList.tsx

+          <NavLink
+            to={`/rooms/${room.id}`}
+            className={getLinkClass}
+            key={index}


Using the array index as a key is generally discouraged in React, as it can lead to performance issues and bugs if the list is ever re-ordered or filtered. It's best practice to use a stable and unique identifier from the data itself. In this case, room.id would be the ideal key.

mateacademy-ai-mentor · 2026-03-30T13:10:53Z

src/client/pages/LoginPage.tsx

+      })
+      .catch((e) => catchError(e, setError));
+
+    formikHelpers.setSubmitting(false);


The setSubmitting(false) call is executed immediately after the userService.createUser promise is initiated, not after it completes. This means the form's submitting state will be reset to false before the API call finishes, which can be misleading for the user. This call should be moved to a .finally() block on the promise chain to ensure it runs only after the asynchronous operation is settled (either successfully or with an error).

mateacademy-ai-mentor · 2026-03-30T13:10:53Z

src/client/services/messageService.ts

+  updateUserData: (id: number, username: string) => {
+    return authClient.put(`/users/${id}`, { username });
+  },


This function updateUserData is related to user management, not messages. It seems misplaced in the messageService. A similar function already exists in userService.ts, making this one redundant. It would be best to remove this function to keep the service focused on its specific domain and avoid code duplication.

mateacademy-ai-mentor · 2026-03-30T13:10:53Z

src/client/services/roomService.ts

+  addMessage: (id: number, messageId: number) => {
+    return authClient.patch(`/rooms/${id}/add-message`, { messageId });
+  },


This addMessage function appears to be unused throughout the client-side application. The functionality for creating new messages is handled by messageService.createMessage. To keep the codebase clean and avoid potential confusion, it would be best to remove this function.

mateacademy-ai-mentor

Review Summary

Changes are needed to fully meet the core requirements. Specifically, messages must display author, time, and text, but the createdAt timestamp is not currently rendered in src/client/components/Room/MessageList/MessageList.tsx — only author and text appear. You should add something like {new Date(message.createdAt).toLocaleTimeString()} next to the message text. Additionally, src/server/app.ts lacks a try...catch around JSON.parse() for WebSocket messages, which risks crashing the entire Node.js process on malformed input. Finally, the migration remove_messages_on_room_delete keeps ON DELETE RESTRICT instead of ON DELETE CASCADE, contradicting its name and intent — either update the constraint or rename the migration to reflect its actual behavior.

✨ 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 · 2026-03-30T18:25:04Z

src/server/app.ts

+  ws.rooms = new Set();
+
+  ws.on('message', (message) => {
+    const data = JSON.parse(message.toString());


Parsing incoming data with JSON.parse is risky without error handling. If a client sends a malformed message (not valid JSON), this line will throw an exception. Because it's inside an event handler without a try...catch block, this unhandled exception will crash the entire Node.js process. It's crucial to wrap this logic in a try...catch block to handle such errors gracefully without affecting other users.

mateacademy-ai-mentor · 2026-03-30T18:25:04Z

src/server/entity/rooms.repository.ts

+const addMessage = async (id: string, messageId: number) => {
+  return db.room.update({
+    where: { id },
+    data: {
+      messages: {
+        connect: { id: messageId },
+      },
+    },
+  });
+};


This addMessage function appears to be unused. The creation of messages and their association with a room is handled within the messagesRepository. It's a good practice to remove dead code to keep the repository focused and easier to maintain.

mateacademy-ai-mentor · 2026-03-30T18:25:04Z

src/client/components/ChatContext.tsx

+  useMemo,
+  useRef,


This migration is named 'remove_messages_on_room_delete' but doesn't change the Message table's foreign key constraint. The constraint remains 'ON DELETE RESTRICT', meaning messages won't be automatically deleted when a room is deleted. Consider changing to 'ON DELETE CASCADE' to match the migration's purpose.

mateacademy-ai-mentor · 2026-03-30T18:25:04Z

prisma/migrations/20260323111204_init/migration.sql

+-- CreateTable
+CREATE TABLE "Room" (
+    "id" TEXT NOT NULL PRIMARY KEY,
+    "name" TEXT NOT NULL


The migration is named 'remove_messages_on_room_delete' but it keeps the same ON DELETE RESTRICT constraint. Messages will NOT be deleted when rooms are deleted - they will remain as orphaned records. If this is intentional to preserve message history per requirements, consider renaming the migration. If CASCADE deletion was intended, the foreign key constraint needs to change to ON DELETE CASCADE.

mateacademy-ai-mentor · 2026-03-30T18:25:05Z

src/client/components/Room/MessageList/MessageList.tsx

+              >
+                <p className="title is-6">{message.author.username}</p>


Messages should display author, time, and text per task requirements. The 'createdAt' field exists in the message data but is not being rendered in the UI.

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

Review Summary

Excellent work on this project! The application is feature-complete, well-structured, and the frontend is particularly robust. I am approving your solution so you can move forward, but I'm doing so for educational purposes as there are a couple of important issues to be aware of for your future projects.

The most critical issue is on the server. Your WebSocket message handler in src/server/app.ts doesn't handle potential errors when parsing incoming messages. If a malformed message is received, JSON.parse will throw an error and crash the entire server. Always wrap JSON.parse in a try...catch block when dealing with external input to ensure your server remains stable.

Additionally, the build is failing due to a linting configuration error. Your package.json specifies ES Modules ("type": "module"), but your .eslintrc.js file uses CommonJS syntax. The simplest fix is to rename the file to .eslintrc.cjs to tell Node.js to treat it as a CommonJS module. You've done a fantastic job building a complex application, keep up the great work!

✨ 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.

brespect

Good progress, but you need to pass all CI check before requesting the review

brespect

Awesome! 🤩

Solution

cb36e5f

mateacademy-ai-mentor suggested changes Mar 30, 2026

View reviewed changes

Solution

69d4bec

alyonashunevych requested a review from mateacademy-ai-mentor March 30, 2026 18:09

mateacademy-ai-mentor suggested changes Mar 30, 2026

View reviewed changes

mateacademy-ai-mentor reviewed Mar 30, 2026

View reviewed changes

Solution

0e64c5b

alyonashunevych requested a review from mateacademy-ai-mentor March 30, 2026 20:56

mateacademy-ai-mentor suggested changes Mar 30, 2026

View reviewed changes

Solution

17d5bb4

alyonashunevych requested a review from mateacademy-ai-mentor March 30, 2026 21:14

brespect suggested changes Mar 31, 2026

View reviewed changes

alyonashunevych added 7 commits March 31, 2026 12:48

Solution

250dfc8

Solution

a676745

Solution

3240287

Solution

5218b85

Solution

e42613c

Solution

485e5e5

Solution

454cee0

alyonashunevych requested a review from brespect March 31, 2026 10:22

brespect approved these changes Apr 1, 2026

View reviewed changes

Conversation

alyonashunevych commented Mar 30, 2026

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!

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!

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!

brespect left a comment

Choose a reason for hiding this comment

Uh oh!

brespect left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants