Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job implementing the core functionality! The form validation and state management work as expected. However, changes are needed to meet the project's technical requirements.
The main issue is the lack of TypeScript. Please add type definitions for component props, state, and event handlers in App.tsx, TodoInfo.tsx, and UserInfo.tsx. This is a core requirement of the task.
Additionally, in TodoInfo.tsx, the application could crash if todo.user is undefined. Please add a conditional check before rendering the UserInfo component to fix this [CHECKLIST ITEM #4].
Lastly, please replace single-letter variable names (e.g., e, t, u) with more descriptive ones like event, todo, and user throughout App.tsx [CHECKLIST ITEM #1].
✨ 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
src/components/TodoInfo/TodoInfo.tsx
Outdated
| export const TodoInfo = () => {}; | ||
| import { UserInfo } from '../UserInfo'; | ||
|
|
||
| export const TodoInfo = ({ todo }) => { |
There was a problem hiding this comment.
Since this is a TypeScript file (.tsx), the component's props should be typed. Defining a type or interface for the todo prop will improve code safety and align with the project's setup.
src/components/TodoInfo/TodoInfo.tsx
Outdated
| > | ||
| <h2 className="TodoInfo__title">{todo.title}</h2> | ||
|
|
||
| <UserInfo user={todo.user} /> |
There was a problem hiding this comment.
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 todo.user property could be undefined if no matching user is found. You should add a check here to ensure todo.user exists before rendering the UserInfo component to prevent the application from crashing.
src/components/UserInfo/UserInfo.tsx
Outdated
| @@ -1 +1,7 @@ | |||
| export const UserInfo = () => {}; | |||
| export const UserInfo = ({ user }) => { | |||
There was a problem hiding this comment.
This component is missing TypeScript types for its props. According to the task requirements, you should use the React TypeScript cheat sheet. The user prop should be properly typed to reflect its shape (e.g., { id, name, username, email }).
| import usersFromServer from './api/users'; | ||
| import todosFromServer from './api/todos'; | ||
|
|
||
| export const App = () => { |
There was a problem hiding this comment.
While the component is in a .tsx file, it's not taking advantage of TypeScript. You should define interfaces for your data structures (like Todo and User) and use them to type your state, props, and event handlers. For example, const App: React.FC = () => { ... } and const [todos, setTodos] = useState<Todo[]>(...). This would make your code more robust and easier to maintain. Please refer to the 'React TypeScript cheat sheet' from the task description.
src/App.tsx
Outdated
| const [todos, setTodos] = useState( | ||
| todosFromServer.map(todo => ({ | ||
| ...todo, | ||
| user: usersFromServer.find(u => u.id === todo.userId), |
There was a problem hiding this comment.
This violates checklist item #1: 'Don't use one-letter variable naming.' Please use a more descriptive name for the parameter, for example, user instead of u.
src/App.tsx
Outdated
| const [title, setTitle] = useState(''); | ||
| const [errors, setErrors] = useState({ title: false, userId: false }); | ||
|
|
||
| const handleSubmit = e => { |
There was a problem hiding this comment.
This violates checklist item #1: 'Don't use one-letter variable naming.' Please rename e to event and provide a proper type for it, like React.FormEvent<HTMLFormElement>.
src/App.tsx
Outdated
| } | ||
|
|
||
| const newTodo = { | ||
| id: todos.length ? Math.max(...todos.map(t => t.id)) + 1 : 1, |
There was a problem hiding this comment.
This violates checklist item #1: 'Don't use one-letter variable naming.' A more descriptive name like todo would be better than t.
src/App.tsx
Outdated
| data-cy="titleInput" | ||
| placeholder="Enter todo title" | ||
| value={title} | ||
| onChange={e => { |
There was a problem hiding this comment.
This violates checklist item #1: 'Don't use one-letter variable naming.' Please rename e to event and give it a type, e.g., React.ChangeEvent<HTMLInputElement>.
src/App.tsx
Outdated
| <select | ||
| data-cy="userSelect" | ||
| value={userId} | ||
| onChange={b => { |
There was a problem hiding this comment.
This violates checklist item #1: 'Don't use one-letter variable naming.' A name like event or changeEvent would be more descriptive than b. Also, consider adding the correct TypeScript type for the event.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on implementing TypeScript across the application! Your progress is excellent. However, this requires changes because a critical issue from the previous review still needs to be addressed.
The application can crash if a user for a specific todo is not found. In src/components/TodoInfo/TodoInfo.tsx, the UserInfo component is rendered without checking if todo.user exists. Since todo.user can be undefined, this will cause an error. Please add a conditional check to only render UserInfo when todo.user is defined, as this violates a core requirement [CHECKLIST ITEM #4].
Once that's fixed, please also rename the single-letter variable u in App.tsx to something more descriptive like user. After these changes, your work should be ready for 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
src/components/TodoInfo/TodoInfo.tsx
Outdated
| > | ||
| <h2 className="TodoInfo__title">{todo.title}</h2> | ||
|
|
||
| <UserInfo user={todo.user} /> |
There was a problem hiding this comment.
The todo.user property can be undefined if a matching user isn't found, but it's being passed to UserInfo, which requires a defined user object. This could cause the application to crash.
This violates checklist item #4: "Don't render the component if the property that you pass to the component has null or undefined value". Please add a conditional check here to only render UserInfo when todo.user exists.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on this update! You've successfully fixed the critical bug by adding the conditional check, which is a fantastic improvement.
I am approving your solution as an educational override so you can move forward. However, please remember to address the minor code style feedback from the last review. In src/App.tsx, the one-letter variable u in your handleSubmit function should be renamed to a more descriptive name like user. This is an important habit for writing clean, readable code [CHECKLIST ITEM #1].
Overall, this is excellent progress. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| title: title.trim(), | ||
| userId, | ||
| completed: false, | ||
| user: users.find(u => u.id === userId), |
There was a problem hiding this comment.
This still uses a one-letter variable u. As mentioned in the previous review, please rename this to a more descriptive name like user to improve readability. This violates checklist item #1: 'Don't use one-letter variable naming'.
No description provided.