React-add-todo-form-Pronina#3564
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is a solid effort, but I'm requesting a few changes to fix some critical bugs. First, the logic for generating a new todo ID fails when the list is empty because Math.max() on an empty array returns -Infinity. This prevents the first todo from being added. Second, the User type is missing the username field, which is a core requirement. Finally, the type assertion as User is unsafe and will cause a runtime error if a user isn't found; please add a check to ensure a user exists before creating the todo. Addressing these points will make your solution robust and 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
src/App.tsx
Outdated
| type User = { | ||
| id: number; | ||
| name: string; | ||
| email: string; | ||
| }; |
There was a problem hiding this comment.
The User type is missing the username field, which is explicitly required by the task description. The description states that the user object on a todo item should contain id, name, username, and email.
src/App.tsx
Outdated
| return; | ||
| } | ||
|
|
||
| const newId = Math.max(...todos.map(todo => todo.id)) + 1; |
There was a problem hiding this comment.
This logic for generating a new ID will fail if the todos array is empty. Math.max() with no arguments returns -Infinity, which will result in an invalid ID for the first todo item. Consider handling the empty array case, for example by providing a default starting value to Math.max.
src/App.tsx
Outdated
|
|
||
| const user = usersFromServer.find( | ||
| currentUser => currentUser.id === Number(userId), | ||
| ) as User; |
There was a problem hiding this comment.
Using as User is an unsafe type assertion. The find method can return undefined if no user matches the userId, which would cause a runtime error when user is used later. It's better to add a check to ensure user is not undefined before proceeding.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the critical issues from the previous review! The application is much more robust now.
However, this requires a couple of changes because the build is currently failing due to a linting error, and there's a violation of a core principle.
- Build Failure: In
src/App.tsx, you have a variable shadowing issue. The parameter in your.find()method is nameduser, which conflicts with theconst useryou declare just outside of it. Please rename the inner parameter to resolve this.const user = usersFromServer.find( u => u.id === todo.userId, // Rename 'user' to 'u' or something similar );
- Code Duplication: The
Usertype is defined in bothsrc/components/TodoInfo/TodoInfo.tsxandsrc/components/UserInfo/UserInfo.tsx. To follow the "Don't Repeat Yourself" principle [CHECKLIST ITEM #2], please move this type to a single shared file (likesrc/types.ts) and import it where needed.
Once these are fixed, your solution will be ready for approval. Keep up the great work!
✨ 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/App.tsx
Outdated
|
|
||
| return { | ||
| ...todo, | ||
| user: user!, |
There was a problem hiding this comment.
Using a non-null assertion (!) here can be risky. If a todo from the server has a userId that doesn't exist in usersFromServer, user will be undefined, and the app will crash when trying to access its properties. A safer approach is to filter out any todos that don't have a matching user, ensuring your application's data is always consistent.
src/components/UserInfo/UserInfo.tsx
Outdated
| type User = { | ||
| id: number; | ||
| name: string; | ||
| username: string; | ||
| email: string; | ||
| }; |
There was a problem hiding this comment.
The User type definition is repeated here and in TodoInfo.tsx. To adhere to the 'Don't Repeat Yourself' (DRY) principle, you should define this type in one central place (for example, in a types.ts file or exported from TodoInfo.tsx) and import it where needed. This violates checklist item #2: "Don't repeat yourself."
src/components/TodoInfo/TodoInfo.tsx
Outdated
| type User = { | ||
| id: number; | ||
| name: string; | ||
| username: string; | ||
| email: string; | ||
| }; |
There was a problem hiding this comment.
This User type is also defined in the UserInfo component. This violates checklist item #2: "Don't repeat yourself. If you see that you copy/paste some block code, maybe, it's time to create a separate variable/function". Consider creating a central types file (e.g., src/types.ts) to store shared type definitions like User and Todo, and then import them in the components that need them.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this task! You've done a fantastic job addressing the previous feedback, especially by creating a centralized types.ts file, which greatly improves the code's maintainability. I am approving your solution so you can move forward, but there is one small style improvement to consider for future tasks.
In src/App.tsx, the variable u is used in a .find() callback. While this works, it's best to use more descriptive names like user to improve code readability, as mentioned in the project guidelines [CHECKLIST ITEM #1].
Overall, this is a very strong submission. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| const u = 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." The variable u should have a more descriptive name, for example, user. This also applies to the parameter u in the .find() method on this line and on line 60.
2 participants
[DEMO LINK](https://ProninaMariia.github.io/react_add-todo-form/