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 really strong submission! You've successfully built a full-stack, real-time chat application that meets almost all the core requirements. The architecture is clean, and the features are well-implemented.
I'm requesting one change because of a critical data integrity bug in the room renaming feature. Once this is fixed, I'll be happy to approve your work.
Required Change
1. Data Inconsistency When Renaming a Room
Location:
src/index.js:84-91
Issue: When a room is renamed, the server correctly moves the message history to the new room name. However, the individual message objects within that history are not updated. Each message still contains theoldNamein itsroomproperty.
Impact: This creates inconsistent data on the server. Any part of the application that relies on a message'sroomproperty will have incorrect information, which is a critical bug.
To Fix: In yourPUT /rooms/:oldNamehandler, after you retrieve the messages for the old room, you'll need to loop through them and update theroomproperty on each message object to thenewNamebefore saving them under the new room name.
Recommended Improvements
I also noticed a few areas where you could make the app even better. These are not required for approval, but are great opportunities for learning and would be expected in a production environment:
- WebSocket Connections: The client creates a new WebSocket connection every time you switch rooms (
client/src/App.jsx). A better approach is to create a single, persistent connection when the app loads to improve performance. - Client-State Updates: Instead of re-fetching the list of rooms from the server after an update (
client/src/App.jsx:72), you can use the data sent in the WebSocket message to update the client's state directly. This is much more efficient. - Unique Message Keys: Using
message.timeas akeyin React (client/src/MessageList.jsx:4) can cause bugs if two messages have the same timestamp. It's best practice for the server to generate a truly unique ID (like a UUID) for each message.
You've done an excellent job here. Just fix that one data bug, and you'll be all set. 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
| "short_name": "React App", | ||
| "name": "Create React App Sample", |
There was a problem hiding this comment.
The short_name and name properties are using the default values from the Create React App template. It's good practice to update these to something more descriptive of your project, like "Chat App" and "Chat with Node.js". This is what users will see if they install the app on their device.
| .field { | ||
| display: grid; | ||
| grid-template-columns: 1fr auto; | ||
| gap: 10px; | ||
| align-items: center; | ||
| } |
There was a problem hiding this comment.
Overriding a common class name from a CSS framework like Bulma is generally not recommended. This global style for .field conflicts with other Bulma features, such as the .has-addons modifier used for the 'Create Room' input, which is designed to remove the gap between elements. To avoid unintended side effects and make the CSS more maintainable, it's better to use a more specific selector or a custom class name for the message input form instead of the generic .field class.
| useEffect(() => { | ||
| socketRef.current = new WebSocket('ws://localhost:3005'); | ||
|
|
||
| socketRef.current.addEventListener('message', (event) => { | ||
| const data = JSON.parse(event.data); | ||
|
|
||
| switch (data.type) { | ||
| case 'message': | ||
| if (data.payload.room === currentRoom) { | ||
| saveData(data.payload); | ||
| } | ||
| break; | ||
|
|
||
| case 'room-update': | ||
| const payload = data.payload; | ||
|
|
||
| fetchRooms(); | ||
|
|
||
| if (payload.action === 'deleted') { | ||
| if (payload.name === currentRoom) { | ||
| setDeletedRoom(currentRoom); | ||
| setCurrentRoom(null); | ||
| } | ||
| } | ||
| if (payload.action === 'renamed' && payload.oldName === currentRoom) { | ||
| setCurrentRoom(payload.newName); | ||
| } | ||
| break; | ||
|
|
||
| default: | ||
| break; | ||
| } | ||
| }); | ||
|
|
||
| return () => { | ||
| socketRef.current.close(); | ||
| }; | ||
| }, [currentRoom]); |
There was a problem hiding this comment.
This useEffect hook, with [currentRoom] in its dependency array, causes a new WebSocket connection to be created every time the user switches rooms. This is highly inefficient and can lead to missed messages during the reconnection period. A WebSocket connection should ideally be established once when the component mounts (using an empty dependency array []). To access the latest currentRoom value inside the event listener's closure, you could store it in a ref which is updated in a separate effect.
| case 'room-update': | ||
| const payload = data.payload; | ||
|
|
||
| fetchRooms(); |
There was a problem hiding this comment.
Calling fetchRooms() here is an unnecessary network request. The WebSocket message payload (data.payload) contains all the information needed to update the client's rooms state directly. For instance, for a 'created' action, you could use setRooms(prevRooms => [...prevRooms, payload.name]). This would make the UI more responsive and reduce server load.
|
|
||
| const deleteRoom = async (name) => { | ||
| await axios.delete(`${API}/rooms/${name}`); | ||
| if (currentRoom === name) setCurrentRoom(null); |
There was a problem hiding this comment.
This line optimistically sets the currentRoom to null if the deleted room is the active one. However, this logic is already handled by the WebSocket 'room-update' event handler (lines 66-70). Having two different places modify state based on the same user action can be problematic. It's better to rely on the WebSocket message as the single source of truth for server-side state changes to ensure consistency across all clients.
| onSubmit={async (event) => { | ||
| event.preventDefault(); | ||
| if (!text.trim()) return; | ||
|
|
||
| await onSend(text, room); | ||
|
|
||
| setText(''); | ||
| }} |
There was a problem hiding this comment.
This onSubmit handler could be made more robust. Currently, if the onSend promise is slow to resolve, a user can click 'Send' multiple times, resulting in duplicate messages. Additionally, if the promise rejects (e.g., a network error), the user receives no feedback. Consider adding a loading state to disable the form during submission and wrapping the await call in a try...catch block to handle potential errors gracefully, for example, by showing an alert to the user.
| export const MessageList = ({ messages }) => ( | ||
| <ul> | ||
| {messages.map(message => ( | ||
| <li key={message.time}> |
There was a problem hiding this comment.
Using message.time as the key is not ideal because it's not guaranteed to be unique. In a scenario where two messages are created at the exact same millisecond, this would result in a duplicate key, which can lead to rendering issues and bugs in React. It's a best practice for the server to assign a truly unique ID (like a UUID) to each message, and that ID should be used for the key prop to ensure stability.
| root.render( | ||
| <App /> | ||
| ); |
There was a problem hiding this comment.
It's a good practice to wrap the root component (App) in <React.StrictMode>. Strict Mode doesn't render any UI but activates additional checks and warnings for its descendants during development, which can help you identify and fix potential bugs related to side effects, deprecated APIs, and other common issues.
| const message = { | ||
| text, | ||
| author, | ||
| room, | ||
| time: new Date().toISOString(), | ||
| }; |
There was a problem hiding this comment.
This message object uses new Date().toISOString() for its time property, which the client then uses as a key. This is not a reliable way to generate a unique key, as two messages could be created in the same millisecond. A better practice is to assign a truly unique ID to each message on the server, for instance, by using a library like uuid or a simple incrementing counter. This ensures React can render lists efficiently and without bugs.
| const messages = rooms.get(oldName); | ||
|
|
||
| rooms.set(newName, messages); | ||
| rooms.delete(oldName); |
There was a problem hiding this comment.
When a room is renamed, this block of code moves the message array to the new room key but introduces a data inconsistency. Each message object within the array still contains the oldName in its room property. This will cause problems if that property is used later. To fix this, you should iterate through the messages and update each message's room property to the newName.
…c/routes/message.route.js
…routes/rooms.route.js
…outes/user.route.js
…src/services/message.service.js
…c/services/rooms.service.js
…/services/user.service.js
ghost
closed this by deleting the head repository
1 participant
No description provided.