Skip to content

add task solution#122

Open
naz4ik wants to merge 3 commits into
mate-academy:masterfrom
naz4ik:develop
Open

add task solution#122
naz4ik wants to merge 3 commits into
mate-academy:masterfrom
naz4ik:develop

Conversation

@naz4ik
Copy link
Copy Markdown

@naz4ik naz4ik commented Jul 23, 2025

No description provided.

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

Hi there! Thanks for your submission. I've reviewed your code, and while you've laid a good foundation for the server, there are several critical issues that need to be addressed before this can be approved. The application is currently not functional due to these problems.

Let's go through the summary of the findings together:

1. Missing Client-Side Application

A key requirement of the task is to build a complete chat application, which means we need both a server and a client. The client-side part is currently missing. Without it, we can't test the user interface, session management (localStorage), or the overall user experience. Please make sure to add the client implementation.

2. Server-Side Issues

There are a few significant problems on the server that break the core functionality.

Database Models and Relationships

  • Incomplete Message Model: Your Message model is missing a roomId field. This is crucial because, without it, there's no way to know which room a message belongs to. This breaks the feature for fetching a room's message history.

    How to fix: Add the roomId to your Message model in src/models/Message.route.js (which should also be renamed to Message.model.js for clarity!):

    // src/models/Message.route.js
    export const Message = client.define('Message', {
      // ...
      roomId: {
        type: DataTypes.INTEGER,
        allowNull: false,
      },
      // ...
    });
  • Broken Room Creation: The createNewRoom function in src/services/room.service.js is trying to create a room by passing a raw string. The Room.create() method expects an object.

    How to fix: Wrap the title in an object:

    // src/services/room.service.js
    const createNewRoom = async (title) => {
      return Room.create({ title }); // Pass an object like this
    };
  • Broken Room Deletion: The deleteRoom function in src/controllers/room.controller.js tries to find a room by roomId, but the primary key column in your Room model is actually named id by default in Sequelize.

    How to fix: Use id to find the room to delete:

    // src/controllers/room.controller.js
    const deleteRoom = async (req, res) => {
      const { roomId } = req.params;
      // ...
      await Room.destroy({ where: { id: roomId } }); // Use 'id' here
      // ...
    };

WebSocket Logic

  • Messages Broadcast to Everyone: Your current WebSocket setup sends every new message to all connected users, not just the ones in the specific room where the message was sent. This means private conversations aren't possible. You'll need to implement logic to track which users are in which rooms and only send messages to the members of that room.

Please focus on fixing these issues. Once they are resolved and the client application is included, your project will be much closer to meeting the requirements. Keep up the effort, you're on your way! 💪


✨ 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

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

Thank you for your hard work! At this stage, your server-side code is much improved, but there are still critical issues: the client-side application is missing (which is required for the task), and the WebSocket logic does not restrict messages to the correct room. Please also fix the HTTP status code in findAllMessages and ensure the argument order in createNewMessage matches the controller. Once these are addressed, your project will be much closer to meeting all requirements! 🚀


✨ 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

const { roomId } = req.params;
const messages = await messagesService.findMessages(roomId);

res.status(201).send(messages);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Issue: The findAllMessages function uses status code 201 (Created) when returning a list of messages. For a GET request, the correct status code should be 200 (OK), not 201. Please change res.status(201).send(messages); to res.status(200).send(messages); to align with REST conventions and likely checklist requirements.

return Message.findAll({ where: { roomId } });
};

const createNewMessage = (roomId, text, userId) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Error: The argument order for createNewMessage is incorrect. In your controller, you call createNewMessage(roomId, userId, text), but here the function is defined as (roomId, text, userId). This will cause the wrong values to be saved in the database. Please update the function signature to (roomId, userId, text) and ensure the object passed to Message.create uses the correct properties.

Comment thread src/index.js
Comment on lines +36 to +40
messageEmitter.on('message', (message) => {
for (const client of wss.clients) {
client.send(JSON.stringify(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.

Issue: The current WebSocket broadcast logic sends every new message to all connected clients, regardless of room. According to the requirements, you should only send messages to clients who are in the same room as the message. You need to implement logic to track client-room associations and filter broadcasts accordingly.

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

Great progress on the server-side implementation! 🎉 Your backend code for handling messages, rooms, and users is well-structured and follows standard RESTful conventions. However, there are a couple of critical issues that need your attention:

  1. Status Code in Message Controller: In findAllMessages, you should return a 200 (OK) status code for successful GET requests instead of 201 (Created). Please update res.status(201).send(messages); to res.status(200).send(messages); to comply with RESTful standards and the task requirements.

  2. Argument Order in Message Service: The createNewMessage function in your service currently expects arguments in the order (roomId, text, userId), but your controller calls it as (roomId, userId, text). This mismatch can cause incorrect message creation. Please update the service function to accept (roomId, userId, text) to match the controller's usage.

Aside from these issues, the rest of your backend logic, including model definitions and router setups, looks solid and ready to go!

Important: The most critical missing piece is the client-side implementation. The requirements specify that users should be able to enter and persist usernames, join/create/rename/delete rooms, and send/receive messages—all via a client application. Currently, your pull request only includes server-side code, so the core functionality is incomplete.

Next Steps:

  • Fix the issues mentioned above in your backend code.
  • Implement and include the client-side application as described in the requirements. Make sure it interacts with your backend using REST and WebSocket as needed.

Keep up the good work! Addressing these points will help you build a fully functional chat application and deepen your understanding of full-stack development. If you need guidance on the client-side, check out resources on localStorage and WebSocket API.

You’re on the right track—just a few more steps to complete the project! 🚀


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

Footnotes

  1. Rate AI review example

const { roomId } = req.params;
const messages = await messagesService.findMessages(roomId);

res.status(201).send(messages);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Issue: The status code for a successful GET request should be 200, not 201. Please change res.status(201).send(messages); to res.status(200).send(messages); as per RESTful conventions and the task requirements.

return Message.findAll({ where: { roomId } });
};

const createNewMessage = (roomId, text, userId) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Error: The argument order for createNewMessage does not match the controller's usage. The controller passes (roomId, userId, text), but the service expects (roomId, text, userId). Please update the service function to accept (roomId, userId, text) and create the message accordingly.

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