Skip to content

Add solution#3575

Open
Kobeliukh wants to merge 3 commits intomate-academy:masterfrom
Kobeliukh:develop
Open

Add solution#3575
Kobeliukh wants to merge 3 commits intomate-academy:masterfrom
Kobeliukh:develop

Conversation

@Kobeliukh
Copy link

Copy link

@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 work on implementing the form functionality, validation, and state management! However, I am requesting changes because the current data structure and flow don't meet the task's core architectural requirements. The main issue is that each todo item must contain a complete user object, not just a userId. This violates a key requirement from the task description.

To fix this, the data preparation logic needs to be moved from child components into the root App component. Specifically:

  1. Update the Todo type in src/types/Todo.ts to include a user property.
  2. In App.tsx, when initializing the todos state and when adding a new todo, find the corresponding user and embed the full user object into the todo item.
  3. Pass the user object down through props (TodoList -> TodoInfo -> UserInfo) instead of the userId.

This will resolve the incorrect data import in UserInfo.tsx and align your solution with the architectural guidelines [CHECKLIST ITEM #5]. You're very close, and these changes will make your solution fully compliant!


✨ 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 +11 to +18
{todos.map(todo => (
<TodoInfo
key={todo.id}
id={todo.id}
title={todo.title}
completed={todo.completed}
userId={todo.userId}
/>

Choose a reason for hiding this comment

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

This component is part of a data flow that violates the task requirements. According to the description, each todo item should include a full user object. Checklist item #5 also requires preparing all data in the App component and passing it down to child components.

Instead of passing just the userId, this component should receive todos that already contain the complete user object and then pass that user object down to the TodoInfo component.

Comment on lines +8 to +21
userId: number;
}

export const TodoInfo = ({ id, title, completed, userId }: Props) => {
return (
<article
data-id={id}
className={clsx('TodoInfo', {
'TodoInfo--completed': completed,
})}
>
<h2 className="TodoInfo__title">{title}</h2>

<UserInfo id={userId} />

Choose a reason for hiding this comment

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

According to the task description, each todo item should contain a user object. Additionally, checklist item #5 states: 'Prepare data in one place (App component) and pass it to child components. Don't use import keyword in non-root components'.

Currently, this component receives a userId and passes it to the UserInfo component, which then has to import all users to find the correct one.

The data should be prepared in the App component. This means the todos array in App's state should contain objects with the full user object nested inside. Then, this component should receive the user object as a prop instead of userId and pass it down to UserInfo.

src/App.tsx Outdated
const allowedChars = /[^\d a-zA-Zа-щА-ЩьЬюЮяЯіІїЇєЄґҐ']/;

export const App = () => {
const [todos, setTodos] = useState<Todo[]>(todosFromServer);

Choose a reason for hiding this comment

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

According to the task description, each todo item should contain a user object. The initial todos state should be prepared by mapping over todosFromServer and adding the corresponding user object to each todo. This aligns with checklist item #5, which advises preparing all necessary data in the root component (App).

Comment on lines +82 to +87
const newTodo: Todo = {
id: maxId,
title: values.title,
completed: false,
userId: values.user,
};

Choose a reason for hiding this comment

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

This new todo object is missing the user property, which is required by the task description (each TODO item must have... a user object...). You'll need to find the selected user from usersFromServer based on values.user and include the full user object here before adding it to the state.

@@ -1 +1,28 @@
export const UserInfo = () => {};
import usersFromServer from '../../api/users';

Choose a reason for hiding this comment

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

This violates checklist item #5: 'Prepare data in one place (App component) and pass it to child components. Don't use import keyword in non-root components'.

This component should not import data directly. Instead, the App component should find the correct user and pass the complete user object down as a prop. This component should then just be responsible for displaying that user's information.

Comment on lines +1 to +6
export interface Todo {
id: number;
title: string;
completed: boolean;
userId: number;
}

Choose a reason for hiding this comment

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

The task description requires each Todo item to have a user object with fields like id, name, username, and email. This interface is missing the user property.

You should add user: User; to this interface and define the User type accordingly, likely in its own file (src/types/User.ts) and import it here.

Copy link

@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 job refactoring the initial data to include the full user object! However, I'm requesting changes because this same logic was missed when creating a new todo.

Currently, the newTodo object created in handleFormSubmit in src/App.tsx is missing the required user property. This is a critical issue that causes the application to crash when a new todo is added. To fix this, you need to find the selected user by their ID (values.user) and embed the full user object into newTodo before adding it to the state.

Once that's fixed, your solution should be complete. You're very close!


✨ 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 +93 to +98
const newTodo: Todo = {
id: maxId,
title: values.title,
completed: false,
userId: values.user,
};

Choose a reason for hiding this comment

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

This newTodo object is missing the required user property.

According to the task description and your Todo type definition, each todo item must include a complete user object. You've correctly done this for the initial list in modifiedTodos. You need to apply the same logic here: find the selected user by their ID (values.user) and embed the full user object into newTodo before adding it to the state.

id={todo.id}
title={todo.title}
completed={todo.completed}
user={todo.user}

Choose a reason for hiding this comment

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

While this correctly passes the user prop, it will cause the application to crash when a new todo is added.

The root cause is in App.tsx. The newTodo object created in the handleFormSubmit function is missing the required user object property. To fix this, you need to find the full user object (based on the selected user ID) and add it to the newTodo object, just like you did for the initial list of todos.

}

export const UserInfo = ({ user }: Props) =>
user && (

Choose a reason for hiding this comment

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

This violates checklist item #4: 'Don't render the component if the property that you pass to the component has null or undefined value.' The conditional check for the user prop should be performed in the parent component (TodoInfo) before rendering UserInfo, rather than inside UserInfo itself. The parent component should be responsible for deciding whether to render this component.

Copy link

@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

Excellent work addressing the previous feedback! The logic for adding a new todo now correctly includes the full user object.

I am approving this solution as an educational override to help you move forward, but there is one critical issue remaining. The application will crash if it tries to render a todo where the user is null. This happens because TodoList.tsx passes todo.user to TodoInfo, which expects a valid User object, not null.

To fix this, you should add a conditional check inside the .map() function in TodoList.tsx to only render <TodoInfo> when todo.user is not null. This follows the guideline that parent components should be responsible for handling null or undefined props [CHECKLIST ITEM #4].

You're very close to a perfect solution, keep up the great work!


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

Footnotes

  1. Rate AI review example

}

export const UserInfo = ({ user }: Props) =>
user && (

Choose a reason for hiding this comment

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

This conditional rendering violates checklist item #4. The check for the user prop's existence should be done in the parent component (TodoInfo). UserInfo should only be responsible for rendering the user's information and should expect to always receive a valid user object. You'll also need to adjust the prop types in both TodoInfo and UserInfo to reflect this change.

Comment on lines +11 to +18
{todos.map(todo => (
<TodoInfo
key={todo.id}
id={todo.id}
title={todo.title}
completed={todo.completed}
user={todo.user}
/>

Choose a reason for hiding this comment

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

The Todo type allows the user property to be null, but the TodoInfo component requires a User object for its user prop. This will cause a type error.

This violates checklist item #4: 'Don't render the component if the property that you pass to the component has null or undefined value.'

You should add a check to ensure TodoInfo is only rendered when todo.user is not null.

id: number;
title: string;
completed: boolean;
user: User;

Choose a reason for hiding this comment

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

The user prop is defined as type User, but the Todo type allows the user property to be null. This creates a type mismatch when the parent component, TodoList, passes a todo with a null user.

According to checklist item #4, the parent component should conditionally render this component only when the user prop is not null. Please adjust the logic in TodoList.tsx to prevent passing null to this component.

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