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 setting up the core structure of the chat application! The real-time messaging and room creation/joining functionalities are working well. However, this submission requires changes because several key requirements from the task description are missing.
Here are the critical issues that need to be addressed:
- Missing Room Management Features: The application must allow users to rename and delete rooms. This functionality is currently not implemented. You'll need to add UI controls on the client-side (
client/src/components/Rooms.tsx) and create corresponding API endpoints on the server (src/index.js) to handle these actions. - Incomplete Message Structure: All messages must include an
author,time, andtext. Thetimeproperty is missing from the messages. The server (src/index.js) should add a timestamp to each message upon receipt before it is stored and broadcast to other clients.
Please implement these missing features to meet all the task requirements. You're on the right track!
✨ 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
| if (!username) { | ||
| return <Login onLogin={setUsername} /> | ||
| } | ||
| if (username) { |
There was a problem hiding this comment.
This if (username) check is redundant. If the code execution reaches this point, the !username condition on line 10 must have been false, meaning username is guaranteed to be a truthy value. You can remove this if statement and its curly braces for cleaner code.
| interface Message { | ||
| author: string; | ||
| text: string; | ||
| } |
There was a problem hiding this comment.
The task requirements state that all messages must have an author, time, and text. The time property is missing from this interface definition.
| const messageData = JSON.stringify({ | ||
| roomId, | ||
| author: username, | ||
| text, | ||
| }); |
There was a problem hiding this comment.
According to the task description, the message object sent to the server must include a time property. You should add a timestamp here before sending the message.
client/src/components/Chat.tsx
Outdated
| <section id="center" className="mt"> | ||
| <div> | ||
| {messages.map((msg, index) => ( | ||
| <div key={index}> |
There was a problem hiding this comment.
Using the array index as a key is not recommended in React, as it can lead to performance issues and bugs with component state when the list changes. A stable, unique identifier from the message data (like a message ID) should be used instead.
| export function Rooms() { | ||
| const [rooms, setRooms] = useState<Room[]>([]); | ||
|
|
||
| const [currentRoom, setCurrentRoom] = useState<string | null>(null); | ||
| const [newRoomName, setNewRoomName] = useState(""); | ||
|
|
||
| const joinRoom = (roomId: string) => { | ||
| setCurrentRoom(roomId); | ||
| } | ||
|
|
||
| useEffect(() => { | ||
| fetch('http://localhost:5000/rooms') | ||
| .then(res => res.json()) | ||
| .then(data => setRooms(data)); | ||
| }, []); | ||
|
|
||
| const handleCreateRoom = (e: React.FormEvent<HTMLFormElement>) => { | ||
| e.preventDefault(); | ||
| if (newRoomName.trim().length < 3) { | ||
| return alert("Room name must be at least 3 characters long"); | ||
| } | ||
|
|
||
| const isExist = rooms.find(r => r.name === newRoomName); | ||
| if (isExist) return alert("Така кімната вже існує!"); | ||
|
|
||
| fetch('http://localhost:5000/rooms', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ name: newRoomName }), | ||
| }) | ||
| .then(res => res.json()) | ||
| .then(savedRoom => { | ||
| setRooms([...rooms, savedRoom]); | ||
| setNewRoomName(''); | ||
| }); | ||
| } | ||
|
|
||
| const name = rooms.find(room => room.id === currentRoom)?.name; | ||
|
|
||
| if (currentRoom) { | ||
| return ( | ||
| <section id="center" className="mt"> | ||
| <h2>Кімната: {name}</h2> | ||
| <Chat roomId={currentRoom} /> | ||
| <button | ||
| type="button" | ||
| className="counter mt" | ||
| onClick={() => setCurrentRoom(null)} | ||
| > | ||
| Вийти з кімнати | ||
| </button> | ||
| </section> | ||
| ) | ||
| } | ||
|
|
||
| return ( | ||
| <section id="center" className="mt"> | ||
| <h2>Кімнати</h2> | ||
| <form onSubmit={handleCreateRoom}> | ||
| <input | ||
| type="text" | ||
| placeholder="Назва кімнати" | ||
| value={newRoomName} | ||
| onChange={(e) => setNewRoomName(e.target.value)} | ||
| /> | ||
| <button | ||
| type="submit" | ||
| className="counter ml" | ||
| > | ||
| Створити | ||
| </button> | ||
| </form> | ||
| <ul> | ||
| {rooms.map(room => ( | ||
| <li key={room.id}> | ||
| {room.name} | ||
| <button | ||
| type="button" | ||
| className="counter ml" | ||
| onClick={() => joinRoom(room.id)} | ||
| > | ||
| Увійти | ||
| </button> | ||
| </li> | ||
| ))} | ||
| </ul> | ||
| </section> | ||
| ) | ||
| } No newline at end of file |
There was a problem hiding this comment.
This component successfully implements room creation and joining. However, the task requirements also specify the ability to rename and delete rooms. This functionality is currently missing.
| app.get('/rooms', (req, res) => { | ||
| res.json(rooms); | ||
| }); | ||
|
|
||
| app.get('/rooms/:id/messages', (req, res) => { | ||
| const roomId = req.params.id; | ||
| const history = messagesHistory[roomId] || []; | ||
|
|
||
| res.json(history); | ||
| }); | ||
|
|
||
| app.post('/rooms', (req, res) => { | ||
| const newRoomName = req.body.name; | ||
| const newRoom = { | ||
| id: Date.now().toString(), | ||
| name: newRoomName, | ||
| }; | ||
|
|
||
| rooms.push(newRoom); | ||
| res.json(newRoom); | ||
| }); |
There was a problem hiding this comment.
The server currently supports creating (POST) and listing (GET) rooms. However, the task requirements also specify the ability to rename and delete rooms. You'll need to implement additional endpoints to handle these actions, for example, using PUT or PATCH for renaming and DELETE for deleting rooms.
| if (!messagesHistory[roomId]) { | ||
| messagesHistory[roomId] = []; | ||
| } | ||
| messagesHistory[roomId].push(parsedData); |
There was a problem hiding this comment.
The task requires all messages to have an author, time, and text. The parsedData object from the client does not include a time property. The server is the ideal place to add this to ensure a consistent timestamp. You should modify the message object here to include a time before pushing it to the history and broadcasting it.
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! I'm approving your solution as you have successfully addressed all the feedback from the previous review. The application is now fully functional, correctly implementing room creation, renaming, and deletion. Furthermore, the server now properly adds a timestamp to each message as required. You've met all the task requirements, great job completing the project!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.