Real-time Chat Application with WebSockets#180
Real-time Chat Application with WebSockets#180Andrew17-2006 wants to merge 6 commits intomate-academy:masterfrom
Conversation
- Add Node.js WebSocket server with rooms and message history - Add username setup with localStorage persistence - Add room management: create, rename, delete - Add glassmorphism UI styling
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on this chat application! You've successfully implemented all the core features, and the UI looks great. I'm requesting a few changes to improve the application's robustness and handle some important edge cases before approval.
There are a few places where the application could crash if it tries to send a message when the WebSocket connection isn't open.
- In
src/MessageForm.jsx, please add a check forws.readyState === WebSocket.OPENbefore callingws.send(). - Similarly, in
src/App.jsx, thecreateRoomandrenameRoomfunctions should have the same check for consistency and stability.
Additionally, there's a potential race condition on the server. In server/index.js, the join_room message can be processed before set_username, causing a user_joined event to be broadcast with an undefined username. Please add a fallback to handle this case, similar to how you handle anonymous messages.
These changes will make your application much more stable. You're very close to a complete solution!
✨ 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
server/index.js
Outdated
| data.roomId, | ||
| { | ||
| type: 'user_joined', | ||
| username: client.username, |
There was a problem hiding this comment.
There's a potential race condition here. The client sends set_username and join_room messages back-to-back, but the server might process join_room before set_username. If that happens, client.username will be undefined. Consider adding a fallback, similar to how you did for the message author on line 118 (client.username || 'Anonymous'), to make this more robust.
src/App.jsx
Outdated
| ws.addEventListener('open', () => { | ||
| ws.send(JSON.stringify({ type: 'set_username', username })); | ||
| ws.send(JSON.stringify({ type: 'join_room', roomId: 'general' })); | ||
| }); | ||
|
|
||
| if (ws.readyState === WebSocket.OPEN) { | ||
| ws.send(JSON.stringify({ type: 'set_username', username })); | ||
| ws.send(JSON.stringify({ type: 'join_room', roomId: 'general' })); | ||
| } |
There was a problem hiding this comment.
This effect adds an 'open' event listener but doesn't have a cleanup function to remove it. This can cause memory leaks if the effect re-runs. It's a good practice to always clean up subscriptions and event listeners. Consider extracting the logic into a handler function and returning a cleanup function from the useEffect that calls ws.removeEventListener.
src/App.jsx
Outdated
| break; | ||
| case 'room_deleted': | ||
| setRooms((prev) => prev.filter((r) => r.id !== data.roomId)); | ||
| setCurrentRoomId((prev) => (prev === data.roomId ? 'general' : prev)); |
There was a problem hiding this comment.
When the current room is deleted, the UI correctly switches to the 'general' room. However, the messages from the deleted room remain visible. To provide a better user experience, consider clearing the old messages and loading the history for the 'general' room. Calling joinRoom('general') might be a good way to handle this.
| function createRoom(roomName) { | ||
| ws.send(JSON.stringify({ type: 'create_room', name: roomName })); | ||
| } | ||
|
|
||
| function renameRoom(roomId, roomName) { | ||
| ws.send(JSON.stringify({ type: 'rename_room', roomId, name: roomName })); | ||
| } |
There was a problem hiding this comment.
Notice that joinRoom and deleteRoom include a check to ensure the WebSocket connection is open before sending data. It would be a good idea to add similar checks to createRoom and renameRoom for robustness and consistency.
src/MessageForm.jsx
Outdated
| if (!text.trim() || !ws) { | ||
| return; |
There was a problem hiding this comment.
This check is good for preventing empty messages, but it doesn't verify if the WebSocket connection is actually open. If the connection is closed or connecting, ws.send() will throw an error. It would be more robust to also check if ws.readyState === WebSocket.OPEN before attempting to send the message.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great progress on the application! However, some changes are needed before this can be approved. The main issue is that the build is failing due to ESLint configuration errors, which prevent the tests from running. The check run logs provide a detailed guide on how to update your .eslintrc.js to resolve this.
Additionally, there is a critical logic issue in src/App.jsx:
- The
useEffecthook for sending the initial username and joining a room has a race condition. You should check ifws.readyStateis already1(OPEN) and send immediately; otherwise, add an'open'event listener to send the messages once the connection is established.
Lastly, a minor improvement for server/index.js:
- In the
ws.on('close')handler, please add a fallback forinfo.usernameto'Anonymous'in case a user disconnects before setting a name.
Fixing the ESLint config and the connection logic will make your application stable and complete. Keep up the great work!
✨ 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/App.jsx
Outdated
| if (ws.readyState !== 1) { | ||
| ws.send(JSON.stringify({ type: 'set_username', username })); | ||
| ws.send(JSON.stringify({ type: 'join_room', roomId: 'general' })); | ||
| } |
There was a problem hiding this comment.
This block of code has a couple of issues that can prevent the initial messages from being sent correctly.
- The condition
ws.readyState !== 1is the opposite of what's needed. You should only send messages when the connection is open (ws.readyState === 1). Attempting to send while the socket is still connecting will result in an error. - There's a race condition. If this
useEffectruns after the socket has already opened, the'open'event listener will be added too late and will never be called, so no messages will be sent in that case.
A more robust approach would be to check ws.readyState. If it's already open, send the messages immediately. Otherwise, add the open event listener.
server/index.js
Outdated
| if (info && info.roomId) { | ||
| broadcastToRoom(info.roomId, { | ||
| type: 'user_left', | ||
| username: info.username, |
There was a problem hiding this comment.
Good job adding the fallback for the username in the join_room handler! However, there's a similar situation here. If a user disconnects before setting a username, info.username will be undefined. Consider adding a fallback to 'Anonymous' here as well for consistency.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! You've successfully addressed all the previous feedback, and the chat application is now stable and fully functional. I am approving your solution so you can continue progressing.
There's just one small improvement related to React best practices in src/App.jsx that I'd like you to consider. Calling joinRoom inside the setCurrentRoomId state updater is a side effect, which should be avoided in state setters as they are expected to be pure functions.
A better approach is to move this logic out of the updater. You can check if the deleted room was the active one and then call joinRoom directly, like this:
case 'room_deleted':
setRooms(prev => prev.filter(r => r.id !== data.roomId));
if (currentRoomId === data.roomId) {
joinRoom('general');
}
break;This is a common point of learning in React, and being mindful of it will make your code cleaner and more predictable. Great job overall, and keep up the fantastic work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| setCurrentRoomId((prev) => { | ||
| if (prev === data.roomId) { | ||
| joinRoom('general'); | ||
| } | ||
|
|
||
| return prev === data.roomId ? 'general' : prev; | ||
| }); |
There was a problem hiding this comment.
Calling joinRoom inside the setCurrentRoomId state updater is a side effect, which should be avoided in state setters. State updater functions are expected to be pure.
You can move this logic out of the updater function. Since currentRoomId is available in this scope, you can check if the deleted room was the active one and call joinRoom directly.
For example:
case 'room_deleted':
setRooms(prev => prev.filter(r => r.id !== data.roomId));
if (currentRoomId === data.roomId) {
joinRoom('general');
}
break;Note that joinRoom already updates the currentRoomId state, so you don't need to do it twice.
A real-time chat application built with Node.js and React.
Features: