Skip to content

solution#130

Open
pryge wants to merge 2 commits into
mate-academy:masterfrom
pryge:develop
Open

solution#130
pryge wants to merge 2 commits into
mate-academy:masterfrom
pryge:develop

Conversation

@pryge
Copy link
Copy Markdown

@pryge pryge commented Sep 4, 2025

Took the idea from another person

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Decision: REQUEST_CHANGES — The current implementation has critical blockers that violate required message schema and validation rules, which will break client interoperability.

Critical issues to fix (blockers):

  1. Message object field names (src/index.js):

    • What: Messages are constructed/emitted with username, message, and timestamp instead of the required author, text, and time.
    • Why: The requirements specify each message must include author, time, and text. Clients will fail if fields don’t match.
    • Where: In the handler that constructs/saves/emits messages (e.g., around lines where you do const msg = { id, username, message, timestamp, roomName } and when emitting history on join).
    • How to fix: Rename fields when constructing and when emitting history:
      • author: username
      • text: message
      • time: timestamp (or set time = Date.now() when creating the message)
      • Ensure stored history uses the same schema so future joins get the correct fields.
  2. Message validation (src/index.js):

    • What: No validation to reject empty or malformed messages.
    • Why: Requirements state empty messages should be rejected and all messages must include author, time, and text.
    • Where: In the message send handler before storing/emitting.
    • How to fix: Validate inputs before saving/emitting:
      • Check that author and text are non-empty strings and room/room name exists.
      • Set time server-side if not provided.
      • If invalid, emit an error (e.g., socket.emit('error', { code: 'INVALID_MESSAGE', message: 'author, text required' })) and do not store/broadcast.
  3. Join non-existent room behavior (src/index.js):

    • What: Joining a room auto-creates it.
    • Why: Requirements indicate joining a non-existent room should be treated as an error; room creation must be explicit.
    • Where: In the join-room handler where you currently create the room if missing.
    • How to fix: If the room does not exist, emit an error (e.g., ROOM_NOT_FOUND) and do not join or create it. Keep room creation in the dedicated create handler.

Minor suggestions (non-blockers):

  • Standardize error payloads for all operations (create/rename/join/delete/message) for consistent client handling.
  • Align internal variable names with the required schema (author, text, time) for clarity.

You’ve built a solid foundation with room management and history handling—great work. Fixing the schema mismatch and adding validation will bring the implementation in line with the requirements and ensure clients work reliably.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/index.js
Comment on lines +46 to +52
const messageData = {
id: Date.now(),
username,
message,
timestamp,
roomName,
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message object constructed here uses { id, username, message, timestamp, roomName }, but the requirements specify that each message must have author (username), time (timestamp), and text (message content) fields. This mismatch will cause issues for clients expecting the required field names. Update the message object to use author, time, and text as field names.

Comment thread src/index.js Outdated
const messageData = {
id: Date.now(),
username,
message,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no validation to ensure that the message text is not empty and that all required fields are present. According to the requirements, empty messages should be rejected and all messages must include author, time, and text fields. Consider adding validation before storing and emitting the message.

Comment thread src/index.js
socket.on('join-room', (data) => {
const { username, roomName } = data;

if (!rooms.has(roomName)) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When handling 'join-room', the server creates the room if it doesn't exist. The requirements suggest that joining a non-existent room should be handled as an error, not by auto-creating the room. Consider emitting an error if the room does not exist, and require explicit room creation.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Decision: APPROVE

Reason: You’ve fully met the core requirements for the multi-room chat system. The server correctly implements room creation, joining (with existence validation), renaming (preserving history), deletion (removing associated messages), and listing. Messages are validated and consistently follow the required schema with server-side timestamps, and the client persists and uses the username from localStorage while rendering complete message history. Error handling and event emissions are appropriately covered, and no critical blockers or missing features were identified.

Minor suggestions (non-blocking):

  • As the project grows, consider modularizing server logic (e.g., separating room/message services) and adding automated tests to lock in behavior.

Great work tightening up validations and aligning everything with the requirements. Nicely done!


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants