Completed#218
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has good structure and correctly handles several features (room creation, joining, deletion, message history display, server-side routing). However, two critical requirements from the task description are missing, which prevents approval: 1. Username persistence via localStorage is not implemented — The task explicitly requires typing a username, sending it to the server, and saving it in localStorage. Currently in src/public/app.js, the author is entered directly in the message form every time, and there is no localStorage usage anywhere. Add a username input field in src/public/index.html and implement save/load logic in app.js. 2. Room rename functionality is missing from the client — While the server has updateRoomHandler (PATCH /rooms/:id) in src/handler/roomsHandlers.ts and the router handles it in src/router.ts, there is no UI button/form to trigger renaming and no corresponding JavaScript function like updateRoomFetch. Add a rename button in the room list and implement the client-side handler. Minor improvements include no user feedback for validation errors (e.g., 409 duplicate room name) and messages do not auto-scroll to show new content. The lock file issue in the CI is an environment/configuration problem, not a code issue. Once you implement the username localStorage persistence and add the room rename UI/handler, the core requirements will be met.
✨ 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
|
|
||
| function renderRooms(data) { | ||
| // clear before append | ||
| [...roomsList.children].forEach((el) => { | ||
| roomsList.removeChild(el); | ||
| }); | ||
|
|
||
| // append data to DOM | ||
| data.forEach((element) => { | ||
| roomsList.insertAdjacentHTML( | ||
| 'beforeend', | ||
| ` | ||
| <li class="rooms-list__item" id='${element.id}'> | ||
| <button data-room='${element.id}'> | ||
| ${element.name} | ||
| </button> | ||
| <button data-delete='${element.id}'>x</button> | ||
| </li> | ||
| `, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Critical: Missing rename room functionality - The renderRooms function (lines 110-130) only renders a join button and delete button for each room. No rename button exists. The server supports PATCH /rooms/:id, but there's no client-side function like renameRoomFetch or UI to invoke it.
| import { randomUUID } from 'node:crypto'; | ||
|
|
||
| export type Message = { | ||
| author: string; | ||
| time: string; | ||
| text: string; | ||
| }; | ||
|
|
||
| export type Room = { | ||
| id: string; | ||
| name: string; | ||
| messages: Message[]; | ||
| }; | ||
|
|
||
| const GENERAL_ROOM_ID = 'general'; | ||
|
|
||
| const rooms: Room[] = [ | ||
| { | ||
| id: GENERAL_ROOM_ID, | ||
| name: 'General', | ||
| messages: [], | ||
| }, | ||
| ]; | ||
|
|
||
| const normalizeRoomName = (roomName: string) => roomName.trim().toLowerCase(); | ||
|
|
||
| const getAllRooms = () => { | ||
| return rooms.map((room) => ({ | ||
| ...room, | ||
| messages: [...room.messages], | ||
| })); | ||
| }; | ||
|
|
||
| const getRoomById = (roomId: string) => { | ||
| const room = rooms.find((item) => item.id === roomId); | ||
|
|
||
| if (!room) { | ||
| return null; | ||
| } | ||
|
|
||
| return { | ||
| ...room, | ||
| messages: [...room.messages], | ||
| }; | ||
| }; | ||
|
|
||
| const hasRoomWithName = (roomName: string, excludeRoomId?: string) => { | ||
| const normalizedRoomName = normalizeRoomName(roomName); | ||
|
|
||
| return rooms.some((room) => { | ||
| if (excludeRoomId && room.id === excludeRoomId) { | ||
| return false; | ||
| } | ||
|
|
||
| return normalizeRoomName(room.name) === normalizedRoomName; | ||
| }); | ||
| }; | ||
|
|
||
| const createRoom = (roomName: string) => { | ||
| const newRoom: Room = { | ||
| id: randomUUID(), | ||
| name: roomName.trim(), | ||
| messages: [], | ||
| }; | ||
|
|
||
| rooms.push(newRoom); | ||
|
|
||
| return { | ||
| ...newRoom, | ||
| messages: [], | ||
| }; |
There was a problem hiding this comment.
Missing: Rename UI elements - The HTML has no input field or button for renaming rooms. The task requires implementing 'rename room' as part of the room operations.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has most of the core functionality working correctly (room CRUD operations, message display, server-side API), but two critical checklist items remain unaddressed: 1. Missing Username Input Form (Checklist Items #1-2) - The task explicitly requires: 'You type a username and send it to the server' and 'It is now username (save it in localStorage)'. Currently: - src/public/index.html has no separate username login form - src/public/app.js has no localStorage logic anywhere - Users must manually type their author name in the message form every session. Required implementation: - Add a username login form in index.html that appears before the chat interface (shown only when no username is saved) - In app.js, implement: - Check localStorage for saved username on page load - If no username exists, show the login form instead of the chat interface - Save username to localStorage when the login form is submitted - Pre-fill the author field from localStorage on subsequent visits. 2. Missing Auto-scroll for Messages (Medium) - After sending a message, the message list should scroll to show the new content. Add messagesList.scrollIntoView({ behavior: 'smooth', block: 'end' }) after the message is appended to the DOM.
✨ 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
| background: var(--color-surface); | ||
| border: 1px solid var(--color-border); | ||
| border-radius: var(--radius-large); | ||
| box-shadow: var(--shadow-panel); | ||
| backdrop-filter: blur(16px); | ||
| } | ||
|
|
||
| .panel--chat { | ||
| min-height: calc(100vh - 4rem); | ||
| grid-template-rows: auto minmax(16rem, 1fr) auto; |
There was a problem hiding this comment.
The task requires users to type a username and save it in localStorage. Currently, the author field is only in the message form (line 69-78) with no localStorage save/load logic. Add a separate username login form in HTML and implement localStorage persistence in app.js.
|
|
||
| const ROOM_MESSAGES_PATHNAME_REGEXP = /^\/rooms\/([^/]+)\/messages$/; | ||
| const ROOM_ID_PATHNAME_REGEXP = /^\/rooms\/([^/]+)$/; | ||
|
|
There was a problem hiding this comment.
Missing localStorage functionality. The task requires 'typing a username and sending it to the server' and 'saving it in localStorage'. There's no localStorage usage in the entire file. The author is entered in the message form every time (lines 193, 199). Consider adding: 1) Check localStorage for saved username on init, 2) Pre-fill the author field or show username login if not set, 3) Save to localStorage when username is provided.
| 'Content-Type': 'application/json', | ||
| }, | ||
| }).then((response) => { | ||
| if (!response.ok) { | ||
| throw new Error('Failed to delete rooms'); | ||
| } | ||
|
|
||
| return response.json(); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
CRITICAL: No username input UI exists. According to the task description and checklist items #1-2, users must type a username and it should be saved to localStorage. Currently, there is only an 'author' field inside the message form that users must fill manually on every page visit. Add a username input/login form in index.html before the chat interface.
|
|
||
| const author = formData.get('author'); | ||
| const text = formData.get('text'); | ||
|
|
||
| if ( | ||
| state.activeRoomId !== null && | ||
| typeof author === 'string' && | ||
| typeof text === 'string' && | ||
| author.trim() !== '' && | ||
| text.trim() !== '' | ||
| ) { |
There was a problem hiding this comment.
CRITICAL: No localStorage implementation for username. The author value is taken directly from formData.get('author') on every message submit, with no save/load logic. According to checklist item #2, the username must be persisted in localStorage. Add: (1) Check localStorage on page load, (2) Show login form if username doesn't exist, (3) Save username to localStorage after submission, (4) Pre-fill the author field on subsequent visits.
|
|
||
| function renderMessage(data) { | ||
| // clear before append | ||
| [...messagesList.children].forEach((el) => { | ||
| messagesList.removeChild(el); | ||
| }); | ||
|
|
||
| if (data.length) { | ||
| data.forEach((element) => { | ||
| messagesList.insertAdjacentHTML( | ||
| 'beforeend', | ||
| ` | ||
| <li class="message-list__item"> | ||
| <div class="message-list__meta"> | ||
| <span class="message-list__author">${element.author}</span> | ||
| <span>${element.time}</span> | ||
| </div> | ||
| <p class="message-list__text">${element.text}</p> | ||
| </li> | ||
| `, | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Minor: Messages don't auto-scroll to the bottom when new messages arrive. Add messagesList.scrollTop = messagesList.scrollHeight after rendering messages to improve UX (mentioned in previous review).
2 participants
No description provided.