Develop by iryna-knyzh · Pull Request #200 · mate-academy/node_chat

iryna-knyzh · 2026-04-23T08:27:34Z

No description provided.

mateacademy-ai-mentor

Review Summary

The implementation successfully covers all core requirements: a full-stack chat application with Node.js/Express server, React client, WebSocket real-time messaging, username submission with localStorage persistence, messages with author/time/text fields, and complete room CRUD operations (create/rename/join/delete). New users properly fetch previous messages when joining a room via the /rooms/:id/messages endpoint. The check run failure was due to a missing package.json in the repository, which is an environment/repository setup issue rather than a code problem with your implementation.

✨ 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-04-23T08:32:00Z

@@ -0,0 +1,33 @@
+import { useState } from 'react';
+import axios from 'axios';
+


Using message.time as the key may cause duplicate key warnings if two messages are sent at the same second. Consider using a more unique identifier like a message index or generated ID.

mateacademy-ai-mentor · 2026-04-23T08:32:00Z

@@ -0,0 +1,17 @@
+export const MessageList = ({ messages }) => (
+  <ul>
+    {messages.map((message) => (


Using message.time as the React key may cause duplicate key warnings if two messages are sent within the same second. Consider using a composite key like ${message.time}-${message.text} or adding a unique identifier to the Message type on the server.

mateacademy-ai-mentor · 2026-04-23T08:32:00Z

@@ -0,0 +1,6 @@
+.field {
+  display: grid;
+  grid-template-columns: 1fr auto;  


Using message.time as a React key works since Date objects serialize with millisecond precision, but this could cause issues if messages are sent in quick succession. Using a unique message ID or index would be more robust practice.

mateacademy-ai-mentor · 2026-04-23T08:32:00Z

@@ -0,0 +1,5 @@
+import { mailer } from './src/utils/mailer.js';


This file imports from ./src/utils/mailer.js which doesn't exist in the codebase. This appears to be unrelated test code for a mailer utility that isn't part of the chat application requirements.

mateacademy-ai-mentor

Review Summary

The implementation covers all core requirements including full-stack architecture, WebSocket real-time messaging, username persistence, message display, and room CRUD operations. However, there's a critical blocker that must be fixed: the ws package is imported in server/src/index.ts but is missing from the project dependencies in pnpm-lock.yaml. This will cause the server to crash immediately on startup since the import will fail. Add ws to the dependencies and regenerate the lock file.

Two medium-priority issues to address when you make that fix:

React keys in MessageList.jsx - Using message.time as a key can cause duplicate warnings when messages are sent within the same millisecond. Consider using a composite key like ${message.time}-${message.username} or adding a unique message ID field.
Missing dependency in App.jsx - The loadRooms function is used in the useEffect dependency array on line 44 but isn't included in the dependency array itself. Wrap it in useCallback or add it to the dependencies to avoid stale closure warnings.

✨ 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-04-23T08:39:29Z

+# See https://help.github.com/articles/ignoring-files/ for more about ignoring files.
+.vscode
+
+# dependencies


Using message.time as the React key can cause duplicate key warnings when multiple messages are sent within the same millisecond. Consider using a combination of fields or a unique identifier for the key.

mateacademy-ai-mentor · 2026-04-23T08:39:29Z

+export const MessageList = ({ messages }) => (
+  <ul>
+    {messages.map((message) => (
+      <li key={message.time}>


Using message.time as the React key can cause duplicate key warnings when messages are sent within the same millisecond. Consider using a combination of fields like ${message.time}-${message.username} or a sequential index for the key.

mateacademy-ai-mentor · 2026-04-23T08:39:29Z

+
+  useEffect(() => {
+    if (username) loadRooms();


The loadRooms function is used as a dependency but not included in the dependency array. This could cause stale closure issues. Consider wrapping loadRooms in useCallback or restructuring the effect.

mateacademy-ai-mentor · 2026-04-23T08:39:29Z

+.field {
+  display: grid;
+  grid-template-columns: 1fr auto;  
+  gap: 10px;


Using message.time as the React key can cause duplicate key warnings when multiple messages are sent within the same millisecond. Consider using a combination of fields or a unique identifier for the key.

mateacademy-ai-mentor · 2026-04-23T08:39:29Z

@@ -0,0 +1,2003 @@
+lockfileVersion: '9.0'
+
+settings:


The ws package is imported but missing from the project dependencies. Add ws to the dependencies in package.json and regenerate the lock file.

mateacademy-ai-mentor

Review Summary

Your chat application implementation is approved. This is an educational override to help you progress while learning. Great work completing all core requirements: the full-stack architecture with Express/TypeScript backend, WebSocket real-time messaging, username persistence, message display with author/time/text, and complete room CRUD operations. The critical ws dependency blocker has been resolved. Two medium-priority React warnings remain: message.time is used as a React key in multiple files (MessageList.jsx, App.jsx, MessageForm.jsx, UsernameForm.jsx, index.jsx) which can cause duplicate key warnings when messages are sent within the same millisecond—consider using a composite key like ${message.time}-${index} or ${message.time}-${message.username}; also, the loadRooms function in App.jsx should be wrapped in useCallback or added to the useEffect dependency array to avoid stale closure warnings. Keep up the excellent progress—these are common React patterns to master!

✨ 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-04-23T08:54:47Z

+## Available Scripts
+
+In the project directory, you can run:


Using message.time as a React key can cause duplicate key warnings when messages are sent within the same millisecond. Consider using a composite key like ${message.time}-${message.username} or ensuring messages have a unique identifier.

mateacademy-ai-mentor · 2026-04-23T08:54:47Z

+
+## Learn More
+
+You can learn more in the [Create React App documentation](https://facebook.github.io/create-react-app/docs/getting-started).


The loadRooms function is used inside the useEffect but is not included in the dependency array. This can cause stale closure warnings. Either wrap loadRooms in useCallback or add it to the dependencies array.

mateacademy-ai-mentor · 2026-04-23T08:54:47Z

+
+const API_URL = `http://${process.env.REACT_APP_API_HOST}:${process.env.REACT_APP_API_PORT}`;


Using message.time as a React key can cause duplicate key warnings when messages are sent within the same millisecond. Consider using a composite key like ${message.time}-${message.username} or adding a unique message ID field.

mateacademy-ai-mentor · 2026-04-23T08:54:47Z

+export const MessageList = ({ messages }) => (
+  <ul>
+    {messages.map((message) => (
+      <li key={message.time}>


Using message.time as a React key can cause duplicate key warnings when multiple messages are sent within the same millisecond. Consider using a composite key like ${message.time}-${index} or adding a unique message ID.

mateacademy-ai-mentor · 2026-04-23T08:54:47Z

+import axios from "axios";
+import "./App.css";


Using message.time as a React key can cause duplicate key warnings when messages are sent within the same millisecond. Consider using a composite key like ${message.time}-${index} or adding a unique message ID field.

mateacademy-ai-mentor · 2026-04-23T08:54:48Z

+
+  useEffect(() => {
+    if (username) loadRooms();


The loadRooms function is used inside the useEffect but is not included in the dependency array. This can cause stale closure warnings in React. Either wrap loadRooms in useCallback with an empty dependency array, or add it directly to the dependencies array.

mateacademy-ai-mentor · 2026-04-23T08:54:48Z

+
+const API_URL = `http://${process.env.REACT_APP_API_HOST}:${process.env.REACT_APP_API_PORT}`;


Using message.time as a React key can cause duplicate key warnings when messages are sent within the same millisecond. Consider using a composite key like ${message.time}-${message.username} or adding a unique message ID field.

mateacademy-ai-mentor · 2026-04-23T08:54:48Z

+import { App } from './App';
+


Using message.time as a React key can cause duplicate key warnings when messages are sent within the same millisecond. Consider using a composite key like ${message.time}-${index} or adding a unique message ID.

iryna-knyzh · 2026-04-23T09:07:46Z

@mateacademy-ai-mentor
you said chat application implementation is approved but its rejected

brespect

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

brespect

Good job, but need some refactor

brespect · 2026-04-24T07:41:38Z

+const API_URL = `http://${process.env.REACT_APP_API_HOST}:${process.env.REACT_APP_API_PORT}`;
+const WS_URL = `ws://${process.env.REACT_APP_API_HOST}:${process.env.REACT_APP_API_PORT}`;


better to store this in config file

brespect · 2026-04-24T07:42:05Z

+function sendMessage(text, roomId) {
+  const username = localStorage.getItem('username');
+  return axios.post(`${API_URL}/messages`, { text, username, roomId });
+}


same way better to create sepparate api handlers for this purpose

brespect · 2026-04-24T07:42:18Z

+import { useState } from 'react';
+import axios from 'axios';
+
+const API_URL = `http://${process.env.REACT_APP_API_HOST}:${process.env.REACT_APP_API_PORT}`;


check same thing everywhere

brespect · 2026-04-24T07:42:55Z

+router.delete('/:id', (req, res) => {
+  const { id } = req.params;
+  if (!rooms[id]) {
+    res.status(404).send({ error: 'Room not found' });
+    return;
+  }
+  delete rooms[id];
+  res.status(204).send();
+});


want to see MVC patter every where, just separate logic into model, view, controller concept

brespect · 2026-04-24T07:44:19Z

+      onSubmit={async (event) => {
+        event.preventDefault();
+
+        await axios.post(`${API_URL}/users`, { username });
+        localStorage.setItem('username', username);
+        onSave(username);
+      }}


don't store a lot of logic in html, move it to handler, check everywhere

brespect

Well done!

iryna-knyzh added 2 commits April 23, 2026 09:38

init project

00f1686

add rooms

a96aa72

mateacademy-ai-mentor suggested changes Apr 23, 2026

View reviewed changes

add package.json

74fc450

iryna-knyzh requested a review from mateacademy-ai-mentor April 23, 2026 08:35

mateacademy-ai-mentor suggested changes Apr 23, 2026

View reviewed changes

fix

c2d8ceb

iryna-knyzh requested a review from mateacademy-ai-mentor April 23, 2026 08:50

mateacademy-ai-mentor suggested changes Apr 23, 2026

View reviewed changes

iryna-knyzh requested a review from mateacademy-ai-mentor April 23, 2026 09:07

brespect suggested changes Apr 23, 2026

View reviewed changes

iryna-knyzh added 2 commits April 24, 2026 09:48

fix

74e9275

fix

347a6d9

iryna-knyzh requested a review from brespect April 24, 2026 06:55

brespect suggested changes Apr 24, 2026

View reviewed changes

fixes after code review

e671d42

iryna-knyzh requested a review from brespect April 28, 2026 08:22

brespect approved these changes Apr 28, 2026

View reviewed changes

		@@ -0,0 +1,33 @@
		import { useState } from 'react';
		import axios from 'axios';

		@@ -0,0 +1,5 @@
		import { mailer } from './src/utils/mailer.js';


		## Learn More

		You can learn more in the [Create React App documentation](https://facebook.github.io/create-react-app/docs/getting-started).


		const API_URL = `http://${process.env.REACT_APP_API_HOST}:${process.env.REACT_APP_API_PORT}`;

		const API_URL = `http://${process.env.REACT_APP_API_HOST}:${process.env.REACT_APP_API_PORT}`;
		const WS_URL = `ws://${process.env.REACT_APP_API_HOST}:${process.env.REACT_APP_API_PORT}`;

Conversation

iryna-knyzh commented Apr 23, 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!

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!

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!

iryna-knyzh commented Apr 23, 2026

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!

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!

brespect left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!