Skip to content

have some troubles with code#194

Open
maximtsyrulnyk wants to merge 12 commits intomate-academy:masterfrom
maximtsyrulnyk:develop
Open

have some troubles with code#194
maximtsyrulnyk wants to merge 12 commits intomate-academy:masterfrom
maximtsyrulnyk:develop

Conversation

@maximtsyrulnyk
Copy link
Copy Markdown

done node chat task

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

I am requesting changes because there are two critical blockers that will prevent the application from running correctly. First, src/server/entity/users.repository.js contains duplicate function definitions (lines 1–36 and 37–74), which causes a JavaScript syntax error due to redeclaring const exports — this must be fixed by removing the duplicate block. Second, in src/client/components/ChatContext.tsx, the WebSocket URL is constructed as ws${import.meta.env.VITE_API_URL}, which produces an invalid URL like wshttp://localhost:3000; it should be ws://${import.meta.env.VITE_API_URL.replace(/^https?:\/\//, '')} to ensure proper protocol handling. These issues directly break core functionality (server startup and real-time messaging), so they must be resolved before approval.


✨ 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

"userId" INTEGER NOT NULL,
"roomId" TEXT NOT NULL,
CONSTRAINT "Message_userId_fkey" FOREIGN KEY ("userId") REFERENCES "User" ("id") ON DELETE RESTRICT ON UPDATE CASCADE,
CONSTRAINT "Message_roomId_fkey" FOREIGN KEY ("roomId") REFERENCES "Room" ("id") ON DELETE RESTRICT ON UPDATE CASCADE
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 ON DELETE RESTRICT clause will prevent a room from being deleted if it contains any messages, which conflicts with the requirement to be able to delete rooms. A better approach would be to use ON DELETE CASCADE, which would automatically delete all messages when their room is deleted. I see you've corrected this in a later migration, which is excellent foresight!


const socketRef = useRef<WebSocket | null>(null);

useEffect(() => {
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 WebSocket URL construction is incorrect. ws${import.meta.env.VITE_API_URL} would result in something like wshttp://localhost:3000 which is an invalid WebSocket URL. It should be ws://${import.meta.env.VITE_API_URL} or the env variable should already include the protocol (e.g., ws://localhost:3000). This will cause the WebSocket connection to fail at runtime.

Comment on lines +37 to +74
};
import { db } from '../utils/db.js';

const getById = async (id) => {
return db.user.findUnique({
where: { id },
});
};

const getByUsername = async (username) => {
return db.user.findUnique({
where: { username },
});
};

const create = async (username) => {
return db.user.create({
data: {
username,
},
});
};

const update = async (id, username) => {
return db.user.update({
where: { id },
data: {
username,
},
});
};

export const usersRepository = {
getById,
getByUsername,
create,
update,
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This file contains duplicate code - the same functions (getById, getByUsername, create, update) and usersRepository export are defined twice (lines 1-36 and 37-74). This causes a syntax error because you cannot redeclare const exports in the same scope. Remove the duplicate code (lines 37-74).

@maximtsyrulnyk
Copy link
Copy Markdown
Author

make changes according reccommendations

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

Good progress on the server-side code and WebSocket connection. However, I am requesting changes because there are a few critical issues that prevent the application from functioning. Once these are addressed, the app should be in a much better state.

Here are the main blockers:

  1. Incomplete ChatContext: In src/client/components/ChatContext.tsx, the context is missing most of the state and functions (currentUser, rooms, selectedRoom, sendMessage, etc.) that other components need. This causes the entire client-side application to crash. Additionally, the onmessage handler needs to be updated to handle different event types from the server (like rooms_updated), not just new messages.

  2. Incorrect API baseURL: In src/client/http/authClient.ts, the baseURL is constructed as http${import.meta.env.VITE_API_URL}, which creates an invalid URL (e.g., httphttp://...) and causes all API requests to fail. Please correct this to use the environment variable directly.

  3. Server Crash on Message Creation: The create function in src/server/entity/messages.repository.js expects a single object argument, but it's being called with three separate arguments from the controller. This signature mismatch causes the server to crash when a user sends a message.

  4. Filename Typo: There's a typo in src/client/components/Room/MessageForm/ndex.ts. It should be renamed to index.ts to work correctly as a barrel file.


✨ 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 on lines +17 to +20
socketRef.current.onmessage = (event) => {
const data = JSON.parse(event.data);
setMessages(prev => [...prev, data]);
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This onmessage handler treats all incoming WebSocket data as a new message to be added to the messages array. However, the server sends different event types (e.g., rooms_updated, room_deleted). You should implement logic here, like a switch statement on data.type, to handle each event type appropriately and update the correct state.

}, []);

return (
<ChatContext.Provider value={{ 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.

The context provides only messages, but other components that use useChat() expect many other values like currentUser, rooms, selectedRoom, and functions like getRoom, sendMessage, and logout. The provider's value needs to be expanded to include all the state and functions required by the rest of the application.


export const DeleteModal = () => {
const navigate = useNavigate();
const { selectedRoom } = useChat();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This component attempts to destructure selectedRoom from the value provided by useChat. As noted in the review for ChatContext.tsx, this value is not currently provided by the context. This will cause a runtime error when this component renders. The ChatContext needs to be updated to provide selectedRoom for this component to work.

export const JoinToRoomModal = () => {
const navigate = useNavigate();
const { setError } = useError();
const { currentUser } = useChat();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You're destructuring currentUser from useChat(), but the ChatContext provider is not currently providing this value. This will cause a runtime error when currentUser.id is accessed in your submit handlers. The ChatProvider needs to be updated to manage and expose the currentUser state.


export const RoomSettingsModal = () => {
const navigate = useNavigate();
const { selectedRoom } = useChat();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This component depends on selectedRoom from the useChat context. However, as noted in the ChatContext.tsx review, the context provider does not currently supply this value. This will cause selectedRoom to be undefined, leading to a runtime error and incorrect navigation logic when selectedRoom.id is accessed. The ChatProvider must be updated to manage and provide the selectedRoom state.


export const Rooms = () => {
const navigate = useNavigate();
const { logout, currentUser } = useChat();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This component relies on logout and currentUser from the useChat hook. However, as noted in the review for ChatContext.tsx, the context provider does not supply these values. This will cause the component to crash at runtime when it tries to destructure them. The ChatProvider needs to be updated to manage and provide this state and functionality.

import { useChat } from '../../ChatContext';

export const RoomsList = () => {
const { rooms, getAllRooms, currentUser } = useChat();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This line attempts to destructure rooms, getAllRooms, and currentUser from the useChat hook. However, the ChatProvider in its current state does not provide these values. This will cause them to be undefined, leading to a runtime error when their properties or methods are accessed (e.g., currentUser.id or getAllRooms()). The ChatContext needs to be updated to manage and provide this state and these functions.

import axios from 'axios';

export const authClient = axios.create({
baseURL: `http${import.meta.env.VITE_API_URL}`,
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 baseURL is being constructed incorrectly. Prepending http to the VITE_API_URL environment variable will likely result in an invalid URL (e.g., httphttp://...), causing all API requests to fail. The environment variable should contain the full base URL, and you should use it directly without adding an extra protocol.

export const LoginPage = () => {
const navigate = useNavigate();
const { setError } = useError();
const { setCurrentUser } = useChat();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This component depends on setCurrentUser from the useChat hook. However, the ChatProvider in its current state does not provide this function. This will cause the component to crash at runtime when the handleSubmit function is called after a successful API request. The ChatContext must be updated to provide setCurrentUser for the application to function correctly.

Comment on lines +15 to +26
const create = async ({ content, userId, roomId }) => {
return db.message.create({
data: {
content,
userId,
roomId,
},
include: {
user: true,
},
});
};
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 signature of this create function expects a single object argument from which it destructures properties (e.g., { content, userId, roomId }). However, in messages.controller.js, it is called with three separate arguments: messagesRepository.create(userId, text, roomId). This mismatch will cause a runtime error.

To fix this, you should update the function signature here to async (userId, content, roomId) to match how it's being used in the controller.

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.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

@maximtsyrulnyk
Copy link
Copy Markdown
Author

edited accotding reccommendations

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.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

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.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

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.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

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.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

@maximtsyrulnyk
Copy link
Copy Markdown
Author

https://github.com/maximtsyrulnyk/node_chat.git edited code according Last Reccommendations

Copy link
Copy Markdown

@Anton-Kuchmasov Anton-Kuchmasov left a comment

Choose a reason for hiding this comment

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

Project doesn't start normally, please rewrite the code (both - FE & BE projects), double-check is everything works as expected and then re-request review

Thread in fe_chat: https://mate.academy/chats/20895?selectedMessageId=10886423

@maximtsyrulnyk
Copy link
Copy Markdown
Author

make changes according last reccommendations

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.

3 participants