Skip to content

Fix checklist issues: guards, labels, handlers naming#3545

Open
gmarchese93 wants to merge 1 commit intomate-academy:masterfrom
gmarchese93:develop
Open

Fix checklist issues: guards, labels, handlers naming#3545
gmarchese93 wants to merge 1 commit intomate-academy:masterfrom
gmarchese93:develop

Conversation

@gmarchese93
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

I am approving your solution because it meets all core functional requirements and there are no critical blockers or broken features. The App component correctly manages todo state, handles form submission via onSubmit, validates both title and user selection, shows and hides error messages at the right times, and creates new todos with the proper shape (including user object and data-id on rendered items). Your child components (TodoList, TodoInfo, UserInfo) are clean, well-typed, and respect the requirement to avoid rendering when their key prop is missing or undefined.

The remaining points are minor and non-blocking: you use a non-null assertion (user!) in prepareTodos, which could be made safer by explicitly handling missing users, and you could tighten component contracts by making props required and relying fully on the root to prepare data. These are improvements for robustness and style rather than correctness, so they don’t prevent this submission from being considered complete. Overall, this is a solid, well-structured implementation that satisfies the task and checklist; nice work.


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

Footnotes

  1. Rate AI review example

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

I am approving this solution because all core functional requirements are implemented correctly: todos are stored and rendered, the add form works with proper validation, new todos include all required fields, IDs are computed as max+1, and UI details like data-cy attributes and error behavior match the spec. The remaining issues are minor checklist/style deviations: in TodoInfo.tsx and TodoList.tsx you guard against missing props inside the child components instead of rendering them conditionally from the parent ([CHECKLIST ITEM #4]), and in App.tsx you use a one-letter variable u in usersFromServer.find(u => u.id === todo.userId) instead of a descriptive name ([CHECKLIST ITEM #1]). These do not break functionality, but in a follow-up you should:

  • Move if (!todo) return null; out of TodoInfo and instead only render <TodoInfo todo={todo} /> when todo is defined, and similarly make todos required in TodoList and ensure App only renders it with a defined array.
  • Rename u to something like user or candidateUser to improve readability and adhere to the checklist.
    Overall, your data flow, typing, and validation logic are solid, and with these small refinements your code will align even better with the project standards.

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

Footnotes

  1. Rate AI review example

Comment on lines +8 to +11
export const TodoInfo = ({ todo }: Props) => {
if (!todo) {
return null;
}

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: 'Do not render a component if the property you pass to the component has null or undefined value (render conditionally outside instead).' The component currently checks if (!todo) return null; (lines below). Instead, render TodoInfo conditionally from the parent (for example in TodoList) so TodoInfo can always assume todo is defined.


const prepareTodos = (todos: typeof todosFromServer): Todo[] => {
return todos.map(todo => {
const user = usersFromServer.find(u => u.id === todo.userId);

Choose a reason for hiding this comment

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

This violates checklist item #1: "Do not use one-letter variable naming." The callback uses a one-letter parameter u in usersFromServer.find(u => u.id === todo.userId). Rename it to a descriptive name such as user or candidateUser to follow the checklist and improve readability.

Comment on lines +8 to +11
export const TodoList = ({ todos }: Props) => {
if (!todos) {
return null;
}

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 component returns null when !todos (lines below). Move this conditional to the parent (App) and make todos a required prop (todos: Todo[]) or ensure App only renders when todos is defined. This keeps data preparation in the root and matches the checklist.

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