react_todo-app by Elber-Kevenny · Pull Request #1268 · mate-academy/react_todo-app

Elber-Kevenny · 2026-02-01T23:19:22Z

mateacademy-ai-mentor

Review Summary

Good work on setting up the application with React Context and a custom hook for localStorage. However, I'm requesting changes because there are several critical bugs and missed requirements that prevent the app from functioning correctly.

Here are the most important issues to address:

Data Loss on Edit: In src/componentes/todolist.tsx, updating a todo is based on the filteredTodo list. This causes any todos hidden by a filter to be permanently deleted. The update logic must always operate on the full todo array from the context.
Incorrect Edit/Save Logic: When a user finishes editing a todo, the title must be trimmed. If the title is empty after trimming, the todo must be deleted. Currently, it's saved with an empty title.
'Toggle All' Button Bug: The active state of the 'Toggle All' button is based on the filtered list, causing it to behave incorrectly when filters are active. Its state must depend on whether all todos are complete.
'Clear Completed' Button: This button is missing a required feature: it must be disabled if there are no completed todos to clear.
Missing Requirements: Please remember to use the classnames library for conditional classes [CHECKLIST ITEM #7] and define constants for filter strings instead of using magic strings [CHECKLIST ITEM #10].

You've built a solid foundation. Addressing these functional issues will get your solution ready for approval.

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-02-01T23:22:03Z

src/componentes/todolist.tsx

+  const context = useContext(TodoContext);
+
+  const [editTitle, setEdiTitle] = useState<string>(''); //elber
+  const [editingId, setEditingId] = useState<number | null>(0);


Initializing editingId with 0 could be problematic if a todo item happens to have an ID of 0. It is safer and more conventional to use null to represent a state where no item is being edited.

mateacademy-ai-mentor · 2026-02-01T23:22:03Z

src/componentes/todolist.tsx

+  const { handleSelected, handleRemove, filteredTodo, setTodo } = context;
+
+  const handleEdit = (id: number, newTitle: string) => {
+    const newArray = filteredTodo.map(t => {


This function has a critical bug. It maps over filteredTodo, which will cause data loss. For example, if you are viewing only 'Active' todos and edit one, setTodo will be called with an array that only contains the active todos, effectively deleting all completed todos from your state.

You should get the full todo array from the context and map over that instead to ensure no todos are lost during an update.

mateacademy-ai-mentor · 2026-02-01T23:22:03Z

src/componentes/todolist.tsx

+            <div
+              key={t.id}
+              data-cy="Todo"
+              className={`todo ${t.completed ? 'completed' : ''}`}


This violates checklist item #7: 'Use classnames lib for calculated classes.' While template literals work, the project requirements specify using the classnames library for conditionally applying CSS classes. This can make your JSX cleaner, especially as conditional logic grows.

mateacademy-ai-mentor · 2026-02-01T23:22:03Z

src/componentes/todolist.tsx

+                  onBlur={() => {
+                    handleEdit(t.id, editTitle);
+                    setEditingId(null);
+                  }}


This onBlur handler is missing two key requirements for editing todos:

The new title must be trimmed of whitespace before saving.

If the title is empty after trimming, the todo must be deleted, not saved with an empty title.

You should add logic here to trim() the editTitle. If the result is an empty string, call handleRemove(t.id). Otherwise, proceed with saving the trimmed title. The same logic needs to be applied to the Enter key press handler.

mateacademy-ai-mentor · 2026-02-01T23:22:03Z

src/componentes/todoApp.tsx

+      <nav className="filter" data-cy="Filter">
+        <a
+          href="#/"
+          className={`filter__link ${filter === 'all' ? 'selected' : ''}`}


The way you've constructed the className violates two important checklist items:

Checklist item solution #7: 'Use classnames lib for calculated classes.' The project requires using the classnames library for conditionally applying CSS classes instead of template literals.

Checklist item Solution #10: 'Do not rely on the unknown string, make constants for this.' Magic strings like 'all', 'active', and 'completed' should be defined in a constants file and imported to avoid typos and make the code easier to maintain. This comment applies to lines 39 and 48 as well.

mateacademy-ai-mentor · 2026-02-01T23:22:03Z

src/App.tsx

+
+    addTodo({
+      id: +new Date(),
+      title: title,


The title is saved without trimming. According to the requirements, you should trim redundant spaces before adding a new todo. This violates checklist item #9: 'NewTodo form should trim redundant spaces.'

mateacademy-ai-mentor · 2026-02-01T23:22:03Z

src/App.tsx

+  useEffect(() => {
+    setIsShowActiveAll(
+      filteredTodo.every(f => f.completed === true),
+    ); /* toda vez que houver uma alteração na
+    dependencia filteredtodo o useefect é ativado e faz a verificação do settIsShowActiveAll
+    every verifica se todos são true, a condição que passei como callback, se todos forem true ele retorna true
+    */
+  }, [filteredTodo]);


The logic to determine if the 'Toggle all' button should be active is based on filteredTodo. This can lead to incorrect behavior when filters are applied. The state of this button should depend on whether all todos are completed, not just the visible ones. This violates task checklist items #5, #6, and #7.

mateacademy-ai-mentor · 2026-02-01T23:22:03Z

src/App.tsx

+          {isShowFooter && (
+            <button
+              type="button"
+              className={`todoapp__toggle-all ${isShowActiveAll ? 'active' : ''}`}


The task requires using the classnames library for conditional classes. Here, a template literal is used instead. Please refactor this to use the classnames library. This violates basic checklist item #7: 'Use classnames lib for calculated classes.'

mateacademy-ai-mentor · 2026-02-01T23:22:03Z

src/App.tsx

+          <TodoContext.Provider
+            value={{
+              todo,
+              setTodo,
+              handleSelected,
+              handleRemove,
+              filteredTodo,
+            }}
+          >
+            <TodoList />
+          </TodoContext.Provider>
        </section>

        {/* Hide the footer if there are no todos */}
-        <footer className="todoapp__footer" data-cy="Footer">
-          <span className="todo-count" data-cy="TodosCounter">
-            3 items left
-          </span>
-
-          {/* Active link should have the 'selected' class */}
-          <nav className="filter" data-cy="Filter">
-            <a
-              href="#/"
-              className="filter__link selected"
-              data-cy="FilterLinkAll"
-            >
-              All
-            </a>
-
-            <a
-              href="#/active"
-              className="filter__link"
-              data-cy="FilterLinkActive"
-            >
-              Active
-            </a>
-
-            <a
-              href="#/completed"
-              className="filter__link"
-              data-cy="FilterLinkCompleted"
+        {isShowFooter && (
+          <footer className="todoapp__footer" data-cy="Footer">
+            <TodoContext.Provider
+              value={{
+                todo,
+                handleRemoveCompleted,
+                filter,
+                handleSelected,
+                handleRemove,
+                handleActive,
+                handleCompleted,
+                handleFilterAll,
+              }}
            >
-              Completed
-            </a>
-          </nav>
-
-          {/* this button should be disabled if there are no completed todos */}
-          <button
-            type="button"
-            className="todoapp__clear-completed"
-            data-cy="ClearCompletedButton"
-          >
-            Clear completed
-          </button>
-        </footer>
+              <TodoApp />
+            </TodoContext.Provider>
+          </footer>


You have two separate TodoContext.Provider instances, one for TodoList and another for TodoApp. This can be simplified by using a single provider that wraps both components. This would centralize the context value and prevent passing unnecessary props, like handleSelected and handleRemove which are not used by TodoApp.

mateacademy-ai-mentor · 2026-02-01T23:22:03Z

src/types/type.ts

+  handleActive: () => void;
+  handleCompleted: () => void;
+  filteredTodo: Todo[];
+  filter: 'all' | 'active' | 'completed';


This union of string literals violates checklist item #10: 'Do not rely on the unknown string, make constants for this.' Instead of using hardcoded strings, you should define an object for your filter constants. Then, you can derive the type from the keys of that object (e.g., keyof typeof FILTERS), which makes your code safer and easier to maintain.

mateacademy-ai-mentor

Review Summary

Excellent progress on this revision! You've successfully implemented several key fixes, like using the classnames library and correctly disabling the 'Clear completed' button. However, I am requesting changes because a few critical bugs remain that block the application from functioning as required.

Please focus on these three main issues:

Data Loss: When editing a todo while a filter is active (src/componentes/todolist.tsx), todos hidden by the filter are permanently deleted. The update logic must map over the full todo array, not filteredTodo.
'Toggle All' Incorrect State: The 'Toggle All' button's status in src/App.tsx is based on the filtered list, causing it to behave incorrectly. It must reflect the state of the entire todo list.
Missing Delete Logic: Editing a todo's title to be empty should delete it, but currently, it's saved as an empty todo. This logic needs to be added to your handleEdit function.

Fixing these core functional bugs is the top priority. Once those are resolved, the app will be in great shape!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-02-02T18:31:04Z

src/componentes/todoApp.tsx

+        <a
+          href="#/"
+          className={classNames('filter__link', {
+            selected: filter === 'all',


To meet the requirement of using constants for filter values ([CHECKLIST ITEM #10]), please use the all constant you've imported from FILTERS for this comparison instead of the hard-coded string.

mateacademy-ai-mentor · 2026-02-02T18:31:04Z

src/componentes/todoApp.tsx

+            selected: filter === 'all',
+          })}
+          data-cy="FilterLinkAll"
+          onClick={() => handleFilterAll(all)}


The handleFilterAll function from the context is defined without any parameters, so passing the all constant here has no effect. Please remove the argument from the function call. This applies to the other filter links as well.

mateacademy-ai-mentor · 2026-02-02T18:31:05Z

src/componentes/todoApp.tsx

+        <a
+          href="#/active"
+          className={classNames('filter__link', {
+            selected: filter === 'active',


As with the 'All' filter, please use the active constant here for the comparison instead of a hard-coded string.

mateacademy-ai-mentor · 2026-02-02T18:31:05Z

src/componentes/todoApp.tsx

+        <a
+          href="#/completed"
+          className={classNames('filter__link', {
+            selected: filter === 'completed',


Please use the completed constant for this comparison to avoid using a magic string.

mateacademy-ai-mentor · 2026-02-02T18:31:05Z

src/App.tsx

+  const handleFilterAll = () => setFilter('all');
+
+  const handleActive = () => setFilter('active');
+
+  const handleCompleted = () => setFilter('completed');


These filter handlers should use the constants from the FILTERS object instead of hard-coded strings. This was correctly done in the TodoApp component's onClick handlers, but the functions here that actually update the state are still using magic strings.

This violates checklist item #10: 'Do not rely on the unknown string, make constants for this.'

mateacademy-ai-mentor · 2026-02-02T18:31:05Z

src/App.tsx

+  useEffect(() => {
+    setIsShowActiveAll(
+      filteredTodo.every(f => f.completed === true),
+    ); /* toda vez que houver uma alteração na
+    dependencia filteredtodo o useefect é ativado e faz a verificação do settIsShowActiveAll
+    every verifica se todos são true, a condição que passei como callback, se todos forem true ele retorna true
+    */
+  }, [filteredTodo]);


The logic for determining if the 'Toggle All' button should be active is incorrect because it depends on filteredTodo. This was a critical issue from the previous review that has not been addressed.

If a user applies a filter (e.g., 'Completed'), this useEffect will only check the visible todos. The button's state must reflect whether all todos in the entire list are completed, not just the filtered ones. The dependency for this effect should be todo, and the logic should be todo.every(...).

mateacademy-ai-mentor · 2026-02-02T18:31:05Z

src/componentes/todolist.tsx

+    const newArray = filteredTodo.map(t => {
+      return t.id === id ? { ...t, title: newTitle } : t;
+    });
+
+    setTodo(newArray); // corrigir


This update logic still contains the critical bug from the previous review. By mapping over filteredTodo, you will cause data loss. For example, if a user filters for "Active" todos and edits one, all "Completed" todos will be permanently deleted because they were not in the filteredTodo array.

To fix this, you must perform the map operation on the full, unfiltered todo array, which you'll need to get from the context.

mateacademy-ai-mentor · 2026-02-02T18:31:05Z

src/componentes/todolist.tsx

+  const handleEdit = (id: number, newTitle: string) => {
+    const newArray = filteredTodo.map(t => {
+      return t.id === id ? { ...t, title: newTitle } : t;
+    });
+
+    setTodo(newArray); // corrigir
+  };


This function is missing a key piece of logic from the requirements: if a todo's title is edited to be an empty string, the todo should be deleted.

Currently, it's just updated with an empty title. You should add a check for an empty newTitle (after trimming) and handle the deletion accordingly.

mateacademy-ai-mentor · 2026-02-02T18:31:05Z

src/types/type.ts

+export type Props = {
+  todos: Todo[];
+  onSelected: () => void;
+  onRemove: () => void;
+};


This Props type definition seems to be a remnant from a previous implementation that might have used prop drilling. Since the application now uses React Context to share state and callbacks, this type is no longer used anywhere and can be safely removed to keep the codebase clean.

mateacademy-ai-mentor

Review Summary

Excellent work on this revision! I am approving this solution as an educational override to help you progress, as you've successfully fixed all the critical bugs from the previous review. The application's state management and core logic are now very solid.

There are just two minor UI details to polish:

In src/componentes/todolist.tsx, the todo completion checkbox is an uncontrolled component. To make it visually update, you need to add the checked property and bind it to the todo's status: checked={t.completed}.
In src/componentes/todoApp.tsx, the item counter text should be pluralized correctly (e.g., 1 item vs. 2 items). You can use a simple conditional to handle this.

These are small fixes on an otherwise great implementation. Fantastic job!

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

Rate AI review example ↩

mateacademy-ai-mentor · 2026-02-02T19:45:28Z

src/componentes/todolist.tsx

+                <input
+                  id={`todo-${t.id}`}
+                  data-cy="TodoStatus"
+                  type="checkbox"
+                  className="todo__status"
+                  aria-label="Marcar como concluido"
+                  onChange={() => handleSelected(t.id)} // em checkbox usamos onchang
+                />


This input is an uncontrolled component because it's missing the checked property. To ensure the checkbox accurately reflects the todo's completion status, you should bind its checked state to t.completed.

mateacademy-ai-mentor · 2026-02-02T19:45:28Z

src/componentes/todoApp.tsx

+  return (
+    <>
+      <span className="todo-count" data-cy="TodosCounter">
+        {`${tamanho} item left`}


The text for the todo count should be pluralized correctly. For example, it should show 1 item left but 0 items left or 2 items left. You can use a simple conditional to change item to items when the count is not equal to 1.

react_todo-app

e08d174

mateacademy-ai-mentor suggested changes Feb 1, 2026

View reviewed changes

add task solution

2ad6363

Elber-Kevenny requested a review from mateacademy-ai-mentor February 2, 2026 18:27

mateacademy-ai-mentor suggested changes Feb 2, 2026

View reviewed changes

new solution

c202324

Elber-Kevenny requested a review from mateacademy-ai-mentor February 2, 2026 19:43

mateacademy-ai-mentor approved these changes Feb 2, 2026

View reviewed changes

ok

15c07dd

Elber-Kevenny requested a review from mateacademy-ai-mentor February 2, 2026 19:53

Conversation

Elber-Kevenny commented Feb 1, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants