Skip to content

add task solution#197

Open
Vladislav-Korsun wants to merge 3 commits into
mate-academy:masterfrom
Vladislav-Korsun:develop
Open

add task solution#197
Vladislav-Korsun wants to merge 3 commits into
mate-academy:masterfrom
Vladislav-Korsun:develop

Conversation

@Vladislav-Korsun
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because there are a few critical issues that will break required functionality and the CI pipeline.

Critical issues that must be fixed:

  • A newly connected client is not sent the current room history, which violates the requirement that “a new user should see all previous messages in the room.” After you create/store the client and set their roomId (e.g. to 'general'), send a room_history message for that room directly to that socket so they immediately see existing messages.
  • In both broadcastRooms() and your room message broadcasting, you check readiness with clientSocket.readyState === clientSocket.OPEN, but OPEN is not defined on the instance, so these comparisons will always fail and messages won’t be sent. Import the WebSocket class from ws and compare against WebSocket.OPEN (or the numeric value 1) instead.
  • In your send_message handler you do const room = rooms.get(clientData.roomId) and then call room.messages.push(...) without checking if room exists; if the room was deleted or never created, this will throw. Add a guard such as if (!room) return; (or send an error back) before mutating room.messages.

Other issues (non-blocking but important):

  • ESLint is failing due to no-console violations around src/index.js:181 and src/index.js:187; remove these console calls or replace them with an approved logging approach, or explicitly disable the rule on those lines only if the logs are truly essential.

You’ve implemented the core room and messaging features well (username handling, message author/time/text, room lifecycle), so once these functional and lint issues are addressed your solution should fully meet the task requirements.


✨ Please rate this review in the Platform's notification center1. 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 button2 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 tag3.
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.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/index.js
Comment on lines +43 to +47
clients.set(socket, {
username: null,
roomId: 'general',
});
broadcastRooms();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After creating and storing the client entry you call broadcastRooms() but you never send the current room history to the newly connected socket. The requirement says a new user should see all previous messages in the room — consider sending the 'room_history' (e.g., for 'general') to the socket right after setting its roomId.

Comment thread src/index.js Outdated
Comment on lines +26 to +27
clients.forEach((clientData, clientSocket) => {
if (clientSocket.readyState === clientSocket.OPEN) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In broadcastRooms() you compare clientSocket.readyState === clientSocket.OPEN. Instances do not provide an OPEN constant, so this will be undefined and the check will fail. Use the OPEN constant from the ws WebSocket class (e.g. import the WebSocket class and compare to WebSocket.OPEN) or compare to the numeric readyState 1.

Comment thread src/index.js Outdated
Comment on lines +75 to +78
clients.forEach((otherClientData, clientSocket) => {
if (
otherClientData.roomId === clientData.roomId &&
clientSocket.readyState === clientSocket.OPEN
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue when broadcasting messages to room members: clientSocket.OPEN is used but not defined on the instance. Use WebSocket.OPEN (imported from 'ws') or the numeric value 1 to check readyState so messages actually get sent.

Comment thread src/index.js
Comment on lines +61 to +73
const clientData = clients.get(socket);
const room = rooms.get(clientData.roomId);
if (!clientData.username) {
return;
}

const chatMessage = {
author: clientData.username,
time: new Date().toLocaleTimeString(),
text: parsedMessage.payload.text,
};

room.messages.push(chatMessage);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You retrieve the room with const room = rooms.get(clientData.roomId) but do not check whether room exists before calling room.messages.push(...). If the room was deleted or missing this will throw. Add a guard (e.g., if (!room) return;) before mutating room.messages.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Your solution is approved because the server-side implementation meets the core functional requirements for the chat app: rooms can be created/renamed/joined/deleted, messages consistently include author, time, and text, new users receive room history on connect/join, and WebSocket readiness checks are used for normal broadcast flows. The server also correctly handles set_username messages and guards against sending messages to non-existent rooms, which aligns with the expected behavior.

There are a couple of robustness issues you should still address: in the delete_room handler (around where you move users back to the general room) you send data to clientSocket without checking clientSocket.readyState === WebSocket.OPEN, and similarly the global error handler’s catch block calls socket.send(...) without a readiness check—both can throw if the socket is already closed. In addition, ESLint reports a no-shadow violation in src/index.js at line 180, column 19 (generalRoom is already declared around line 51, column 9); you can fix this by renaming the inner generalRoom variable (for example to generalRoomState or similar) or by removing the redeclaration and reusing the outer variable. On the client side, please make sure (if you haven’t already) that the username is captured via the UI, stored in localStorage, and sent to the server as a set_username message so it matches how the server expects to receive it.

Overall, the main functional and architectural requirements are satisfied, and the remaining items are refinements that will make your solution more robust and keep CI green once you fix the linter warning.


✨ Please rate this review in the Platform's notification center1. 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 button2 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 tag3.
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.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution as an educational override so you can keep progressing while you address a couple of remaining robustness issues.

You implemented the core chat features well: rooms can be created/renamed/joined/deleted, messages include author, time, and text, room history is sent on connect/join, and you already use WebSocket.OPEN readiness checks in your normal broadcast paths, which shows good attention to reliability. The two critical items to fix are: (1) in src/index.js, in the delete_room handler where you move users back to the default room and loop over clients to clientSocket.send(...), you need to guard each send with if (clientSocket.readyState === WebSocket.OPEN) { ... } to avoid throwing when a socket is already closed [CHECKLIST ITEM #1]; and (2) in the global error handler catch block in src/index.js where you call socket.send(...), you should first check if (socket.readyState === WebSocket.OPEN) before sending the error message, for the same reason [CHECKLIST ITEM #2]. As a minor improvement, double-check that your client code captures the username via the UI, stores it in localStorage, and sends a { type: 'set_username', payload: { username } } message so the server-side username handling you added is fully used. Overall, your server structure, room management, and history handling are solid, and tightening these last readiness checks will make your chat app much more robust—nice work getting this far.


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

Footnotes

  1. Rate AI review example

Comment thread src/index.js

const defaultRoom = rooms.get('general');

clientSocket.send(
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When moving users back to the default room you call clientSocket.send(...) without verifying the socket is open. Add a readiness check like if (clientSocket.readyState === WebSocket.OPEN) { ... } before sending to avoid throws on closed sockets (this is required).

Comment thread src/index.js
broadcastRooms();
}
} catch (error) {
socket.send(
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the catch block you call socket.send(...) unconditionally. Check socket.readyState === WebSocket.OPEN before sending the error message to avoid throwing when the socket is already closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants