Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is a great start on the chat application, and the core functionality for creating, joining, and deleting rooms is working well.
However, I am requesting changes because a key requirement from the task description is missing. The application needs to support the ability for users to rename rooms. This feature is currently absent from both the client and server.
You will need to update client/chat-client/src/App.tsx to include a UI for renaming and server/index.js to handle the rename logic. Once this functionality is implemented, the project will be complete. You're very close!
✨ 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
| <button onClick={() => joinRoom(r)}>{r}</button> | ||
| <button onClick={() => deleteRoom(r)}>x</button> | ||
| </div> |
There was a problem hiding this comment.
The task requirements mention the ability to 'create / rename / join / delete' rooms. The functionality to rename a room appears to be missing from the client-side implementation.
| switch (message.type) { | ||
| case 'join': | ||
| ws.username = message.username; | ||
| ws.room = message.room || 'general'; | ||
|
|
||
| if (!rooms[ws.room]) { | ||
| rooms[ws.room] = []; | ||
| } | ||
|
|
||
| ws.send( | ||
| JSON.stringify({ | ||
| type: 'history', | ||
| messages: rooms[ws.room], | ||
| }), | ||
| ); | ||
| break; | ||
|
|
||
| case 'message': | ||
| if (!ws.room) return; | ||
|
|
||
| const msg = { | ||
| author: ws.username, | ||
| text: message.text, | ||
| time: new Date().toISOString(), | ||
| }; | ||
|
|
||
| if (!rooms[ws.room]) { | ||
| rooms[ws.room] = []; | ||
| } | ||
|
|
||
| rooms[ws.room].push(msg); | ||
|
|
||
| wss.clients.forEach((client) => { | ||
| if ( | ||
| client.readyState === WebSocket.OPEN && | ||
| client.room === ws.room | ||
| ) { | ||
| client.send( | ||
| JSON.stringify({ | ||
| type: 'message', | ||
| message: msg, | ||
| }), | ||
| ); | ||
| } | ||
| }); | ||
| break; | ||
|
|
||
| case 'create_room': | ||
| if (!rooms[message.name]) { | ||
| rooms[message.name] = []; | ||
| } | ||
| break; | ||
|
|
||
| case 'delete_room': | ||
| delete rooms[message.name]; | ||
| break; | ||
| } |
There was a problem hiding this comment.
The task requires the ability to create, rename, join, and delete rooms. The server logic for handling a 'rename' operation is missing from this switch statement.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job implementing the room renaming feature! You've successfully addressed the main point from the last review, and the core server-side logic for room management is in place. However, this requires changes because a critical piece of functionality is missing: the server doesn't notify clients when the list of rooms changes. This means that after a room is created, deleted, or renamed, the change is not reflected in any other user's UI, which breaks the multi-user experience. To fix this, please update server/index.js to broadcast the updated list of rooms to all connected clients after handling the create_room, delete_room, and rename_room messages. You should also send the full room list to a client as soon as they connect. You're very close to finishing!
✨ 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
|
|
||
| wss.on('connection', (ws) => { | ||
| console.log('User connected'); | ||
|
|
There was a problem hiding this comment.
It's good practice to send the new user the list of available rooms as soon as they connect. This ensures their UI has the complete room list from the very beginning.
server/index.js
Outdated
| case 'create_room': | ||
| if (!rooms[message.name]) { | ||
| rooms[message.name] = []; | ||
| } |
There was a problem hiding this comment.
After creating a new room, you need to broadcast the updated list of rooms to all connected clients. Otherwise, only the user who created it will know it exists. The client is already set up to handle a message with type: 'rooms'.
| break; | ||
|
|
||
| case 'delete_room': | ||
| delete rooms[message.name]; |
There was a problem hiding this comment.
Similar to creating a room, after deleting one, you must broadcast the updated room list to all clients so the deleted room is removed from their UIs.
| client.room = newName; | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
After all the rename logic is complete, you should broadcast the new, updated list of rooms to all clients. This will ensure everyone's room list is synchronized with the server.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! You've successfully implemented the broadcasting of room changes, which was the main goal. Your application now meets all the core requirements.
I am approving this submission as an educational override to help you progress, but there are a couple of important multi-user synchronization issues to be aware of for a truly robust application:
- Room Deletion: When a room is deleted, users inside it aren't moved to a default room. This can cause them to get "stuck" in a non-existent room. A good fix would be to move any users from a deleted room back to 'general' on the server.
- Room Renaming: If a user is in a room that gets renamed by someone else, their client UI doesn't update to show the new name. The server should send a specific notification to clients in the affected room so they can update their state.
Additionally, consider preventing the 'general' room from being deleted or renamed to ensure there's always a fallback. You've built a solid foundation, and addressing these final edge cases will make your chat application flawless. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| if (room === roomName) { | ||
| setRoom('general'); | ||
| } |
There was a problem hiding this comment.
This correctly updates the UI for the user who initiated the deletion. However, this logic doesn't account for when another user deletes the room you're currently in. In that scenario, your client's room state won't change, leaving you in a non-existent room.
A good way to fix this is to add a check inside your ws.onmessage handler. When you receive an updated rooms list, you can verify if your current room is still present in that list. If not, you should switch to the 'general' room.
| if (room === oldName) { | ||
| setRoom(newName); | ||
| } |
There was a problem hiding this comment.
Similar to the deleteRoom function, this optimistic update only works for the user performing the rename. If another user renames the room you are currently in, your client's room state will not be updated. This will cause your UI to show the old room name, even though your messages are being sent to the new room, which can be confusing.
A complete solution for this would likely require a change on the server to send a specific notification to all clients within the renamed room, telling them to update their state.
| break; | ||
|
|
||
| case 'delete_room': | ||
| delete rooms[message.name]; |
There was a problem hiding this comment.
It's a good idea to prevent the 'general' room from being deleted, as it serves as a fallback. You can add a check here, like if (message.name === 'general') return;.
Also, consider what happens to users who are currently in the room that is being deleted. Their client.room property will still point to the now-deleted room. If they send a message, the room will be recreated. A better approach is to iterate through all connected clients and move any user from the deleted room back to 'general'.
| delete rooms[oldName]; | ||
|
|
||
| wss.clients.forEach((client) => { | ||
| if (client.room === oldName) client.room = newName; |
There was a problem hiding this comment.
This logic correctly updates the server's internal state for which room each client is in. However, the client's UI doesn't get updated. A user who is in a room when it gets renamed will still see the old room name, even though their messages are being sent to the new one.
To fix this, you could send a specific message to the clients in the affected room, telling them to update their room state to the newName.
2 participants
No description provided.