Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on this chat application! The project is well-structured and successfully implements almost all the required features. However, I'm requesting changes due to a critical bug that affects core functionality.
Required Fix:
- Username is lost on server after page reload: When the page is reloaded, the client correctly loads the username from
localStorage, but it doesn't re-send this information to the server. As a result, the server considers the user to be 'Anonymous', and any messages sent after a reload are incorrectly attributed. To fix this, please ensure the'user:set'event is emitted to the server whenever the username is loaded, both on initial login and on page refresh.
I've also noticed a couple of areas for improvement that you can consider:
- In
app.jsx, auseEffecthook has a stale closure risk. It's good practice to wrap functions used inside effects (likejoinRoom) withuseCallbackand include them in the dependency array. - The delete confirmation modal could be more user-friendly by disabling the 'Confirm' button until the user types something, as the placeholder text suggests.
Once the main bug with the username is fixed, your solution will be ready for approval. 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/public/app.jsx
Outdated
| useEffect(() => { | ||
| socket.on('rooms:list', (list) => { | ||
| setRooms(list); | ||
| }); | ||
|
|
||
| socket.on('room:history', ({ roomId, messages: msgs }) => { | ||
| setMessages((prev) => ({ ...prev, [roomId]: msgs })); | ||
| }); | ||
|
|
||
| socket.on('message:new', ({ roomId, message }) => { | ||
| setMessages((prev) => ({ | ||
| ...prev, | ||
| [roomId]: [...(prev[roomId] || []), message], | ||
| })); | ||
| }); | ||
|
|
||
| socket.on('room:deleted', (roomId) => { | ||
| setCurrentRoomId((prev) => (prev === roomId ? null : prev)); | ||
| setMessages((prev) => { | ||
| const copy = { ...prev }; | ||
| delete copy[roomId]; | ||
| return copy; | ||
| }); | ||
| }); | ||
|
|
||
| socket.on('room:created', ({ id }) => { | ||
| joinRoom(id); | ||
| }); | ||
|
|
||
| return () => { | ||
| socket.off('rooms:list'); | ||
| socket.off('room:history'); | ||
| socket.off('message:new'); | ||
| socket.off('room:deleted'); | ||
| socket.off('room:created'); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
This effect is set to run only once on component mount because of the empty dependency array []. However, it uses the joinRoom function, which is defined within the component's render scope. This can lead to issues with "stale closures".
While it works in this case, it's a fragile pattern. It's better to wrap functions like joinRoom in useCallback and include them in the dependency array to make dependencies explicit and prevent potential bugs.
src/public/app.jsx
Outdated
| function handleLogin(name) { | ||
| localStorage.setItem('chat_username', name); | ||
| setUsername(name); | ||
| socket.emit('user:set', name); | ||
| // Auto-join general room | ||
| joinRoom('general'); | ||
| } |
There was a problem hiding this comment.
The 'user:set' event is only sent when a user logs in manually. When the page is reloaded, the username is correctly loaded from localStorage, but this event is not sent to the server. As a result, any messages sent after a reload will have 'Anonymous' as the author.
To fix this, consider using a useEffect hook that emits 'user:set' whenever the username state is populated, both on initial login and on subsequent page loads.
| {modal?.type === 'delete' && ( | ||
| <Modal | ||
| title={`Delete "${modal.room.name}"?`} | ||
| placeholder="Type anything to confirm..." | ||
| onConfirm={() => handleDeleteRoom(modal.room)} | ||
| onCancel={() => setModal(null)} | ||
| /> |
There was a problem hiding this comment.
The placeholder text 'Type anything to confirm...' suggests the user must type to enable the confirm button. However, the button is always enabled, allowing deletion without typing. This could lead to accidental deletions.
To improve this, you could disable the 'Confirm' button in the Modal component until the input field is not empty, similar to how the 'Join Chat' button is disabled in the LoginScreen.
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 submission! You've successfully resolved the critical bug where the username was lost after a page reload by correctly synchronizing the user state with the server. It's great to see that you also implemented the optional suggestions, which improves the overall quality and user experience. The application now meets all requirements and functions perfectly. I'm happy to approve this.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.