Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work setting up the server with Express and WebSockets! This is a solid foundation for a real-time application. However, this submission requires changes because several key features from the requirements are missing.
Here are the main points to address:
- Missing Message Author: All messages, whether sent via HTTP POST or WebSocket, must include an
authorfield. The server needs to be updated to receive and store the author's name with each message. - No Message History for New Users: When a new user connects, they should receive all previous messages for the room they join. Currently, a new client only sees messages sent after they've connected.
- Missing Room Functionality: The application lacks the concept of chat rooms. The core functionality to create, rename, join, and delete rooms needs to be implemented.
✨ 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
src/index.js
Outdated
| const message = { | ||
| text, | ||
| time: new Date(), | ||
| }; |
There was a problem hiding this comment.
The requirements state that all messages must have an author, time, and text. This message object is missing the author. The server needs to receive the author's name from the client and include it here.
src/index.js
Outdated
| emmiter.once('message', (message) => { | ||
| res.status(200).send(messages); | ||
| }); |
There was a problem hiding this comment.
This endpoint uses long polling by waiting for a 'message' event. For a user to retrieve the message history, it would be better to send the messages array immediately. The current implementation forces the client to wait for a new message to be sent before it can get any of the previous ones.
src/index.js
Outdated
|
|
||
| const wss = new WebSocketServer({ server }); | ||
|
|
||
| wss.on('connection', (connection) => { |
There was a problem hiding this comment.
One of the requirements is that a new user should see all previous messages. When a new client connects, you should send them the existing messages array. Currently, a new client will only see messages sent after they've connected.
src/index.js
Outdated
| const message = { | ||
| text: text.toString(), | ||
| time: new Date(), | ||
| }; |
There was a problem hiding this comment.
Similar to the POST endpoint, messages received via WebSocket are also missing an author. The requirements state that all messages must include an author. The data received from the client should likely be a JSON string containing both the text and the author.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent progress on the chat server! You've successfully implemented all the required HTTP endpoints for managing rooms. However, some changes are needed in the WebSocket implementation to fix a couple of critical issues.
First, the connection.on('room_join', ...) event handler will not work as intended because the ws library handles all incoming data through the 'message' event. You'll need to move your room-joining logic inside the connection.on('message', ...) handler and parse the incoming message to determine the user's intent.
Second, when a room is renamed or deleted via the HTTP API, the connected WebSocket clients are not updated. This leaves them in a stale state, connected to a room that no longer exists, and they will stop receiving messages. You need to find a way to notify these clients of the change.
✨ 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
src/index.js
Outdated
| rooms[newName] = rooms[name]; | ||
| delete rooms[name]; |
There was a problem hiding this comment.
When a room is renamed, you should also update the room property for all connected WebSocket clients that are currently in that room. Otherwise, they will be stuck in a room that no longer exists and won't receive new messages.
|
|
||
| app.delete('/rooms/:name', (req, res) => { | ||
| const { name } = req.params; | ||
| delete rooms[name]; |
There was a problem hiding this comment.
When a room is deleted, what happens to the users currently in it? Their client.room property will refer to a room that no longer exists. Consider notifying these clients or moving them to a default room.
src/index.js
Outdated
| connection.on('room_join', (room) => { | ||
| connection.room = room; | ||
| if (rooms[room]) { | ||
| connection.send(JSON.stringify({ history: rooms[room] })); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This room_join event handler will not be triggered by messages sent from the client. All incoming data from a WebSocket client triggers the 'message' event. You need to handle different types of client requests (like joining a room vs. sending a message) inside the 'message' event handler, for example by checking a type property in the JSON payload.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the feedback from the previous review! The WebSocket logic for joining rooms and notifying clients about deleted rooms is now working correctly.
The only missing piece is the ability to rename a room, as required by the task description. To implement this, you could add a PUT or PATCH endpoint like /rooms/:name.
I am approving your solution as an educational override to help you progress. You've built a solid foundation here, keep up the excellent work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| ensureRoom(name); | ||
| res.status(201).send({ name }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
According to the task description, you need to implement the ability to 'create / rename / join / delete' rooms. The functionality to rename a room is missing. You should add an endpoint here, for example, PUT /rooms/:name, to handle renaming.
2 participants
No description provided.