Implement Todo App features by eg3844540-svg · Pull Request #1328 · mate-academy/react_todo-app

eg3844540-svg · 2026-07-04T19:50:00Z

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are still a few explicit checklist requirements not met, which makes this a blocker for completing the task as specified. The biggest issue is the repeated use of raw filter strings ('All', 'Active', 'Completed') throughout Footer.tsx, TodoList.tsx, TodoContext.tsx, TodoProvider.tsx, and Filter.ts instead of a shared FILTERS constants object; this violates the “do not rely on unknown string” rule [CHECKLIST ITEM #10] and must be fixed by defining FILTERS and using those values everywhere you compare or set filter. A second critical issue is that conditional class names are built via ternaries/template strings in Footer, TodoHeader, and TodoItem; the task explicitly requires using the classnames library for calculated classes [Basic React CHECKLIST ITEM #7], so you need to import classnames and refactor those className expressions to use it (for example, classNames('todoapp__toggle-all', { active: todos.length > 0 && todos.every(...) })). There is also a requirement nuance around showing only the NewTodo form when the todos array is empty [CHECKLIST ITEM #12]; you already hide TodoList and Footer when todos.length === 0, so just make sure the header in that state renders only the new-todo input and no other controls consistently.

Functionally your app is very solid: context, persistence, filtering, toggle-all, clear-completed, trimming, and inline editing all behave correctly, and your components and types are well structured. Once you centralize filter values into constants, adopt classnames for conditional class handling, and ensure the empty-state UI matches the “only NewTodo form” requirement, your solution will fully meet both the behavior and checklist expectations. Keep going—you're very close, and these changes are mostly about aligning with the exact conventions the task is testing.

✨ 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-07-04T19:52:00Z

+          href="#/"
+          data-cy="FilterLinkAll"
+          className={
+            filter === 'All'


This violates checklist item #10: "Do not rely on the unknown string, make constants for this." Here, filter === 'All' and onClick={() => setFilter('All')} rely on raw string values instead of a shared FILTERS constants object.

mateacademy-ai-mentor · 2026-07-04T19:52:00Z

+        <a
+          href="#/"
+          data-cy="FilterLinkAll"
+          className={


This violates checklist item #7 under Basic React: "Use classnames lib for calculated classes." The conditional class is built manually with a ternary string; consider using the classnames library as required.

mateacademy-ai-mentor · 2026-07-04T19:52:00Z

+          href="#/active"
+          data-cy="FilterLinkActive"
+          className={
+            filter === 'Active'


This violates checklist item #10: "Do not rely on the unknown string, make constants for this." filter === 'Active' and setFilter('Active') should use a shared FILTERS.active constant instead of hardcoded strings.

mateacademy-ai-mentor · 2026-07-04T19:52:00Z

+          href="#/completed"
+          data-cy="FilterLinkCompleted"
+          className={
+            filter === 'Completed'


This violates checklist item #10: "Do not rely on the unknown string, make constants for this." filter === 'Completed' and setFilter('Completed') should use a shared filter constants object.

mateacademy-ai-mentor · 2026-07-04T19:52:00Z

+    setTimeout(() => {
+      (
+        document.querySelector(


This violates checklist item #12: "Show only NewTodo form if todos array is empty." Here, the footer (which includes filters and clear button) is only rendered when todos.length > 0; you should adjust the layout so that when todos is empty, only the new-todo field is shown, and when todos is not empty, the list and footer are visible as in the reference app.

mateacademy-ai-mentor · 2026-07-04T19:52:00Z

+export const TodoContext = createContext<TodoContextType>({
+  todos: [],
+  setTodos: () => {},
+  filter: 'All',


Here the default filter value is the raw string 'All', which also violates checklist item #10: "Do not rely on the unknown string, make constants for this." Consider importing and using FILTERS.all instead.

mateacademy-ai-mentor · 2026-07-04T19:52:00Z

+    return savedTodos ? JSON.parse(savedTodos) : [];
+  });
+
+  const [filter, setFilter] = useState<Filter>('All');


This violates checklist item #10: "Do not rely on the unknown string, make constants for this." The filter is initialized with the raw string 'All'; you should use a shared FILTERS constant (e.g. FILTERS.all) rather than a magic string.

mateacademy-ai-mentor · 2026-07-04T19:52:00Z

@@ -0,0 +1,35 @@
+import React, { useEffect, useState } from 'react';


Checklist item #10 requires filter values to come from a constants object. Here Filter is a union of string literals 'All' | 'Active' | 'Completed' without corresponding constants; introduce a FILTERS object and align the type with those values to avoid magic strings.

mateacademy-ai-mentor · 2026-07-04T19:52:00Z

+    const savedTodos = localStorage.getItem('todos');
+
+    return savedTodos ? JSON.parse(savedTodos) : [];
+  });


This also relies on the raw string 'All' for the filter default in context, which violates checklist item #10: "Do not rely on the unknown string, make constants for this." Use a shared filter constant instead.

mateacademy-ai-mentor · 2026-07-04T19:52:00Z

@@ -0,0 +1 @@
+export type Filter = 'All' | 'Active' | 'Completed';


Checklist item #10 also applies here: Filter type is currently a union of string literals. Once you introduce FILTERS constants, ensure that this type is either derived from those constants or that all usages reference the constants instead of raw strings.

mateacademy-ai-mentor

Review Summary

I am approving your solution because all critical requirements and checklist items are now satisfied, including context-based state management, localStorage persistence, filtering, toggle-all, clear-completed, and inline editing behavior. The remaining note about filter constants in src/context/TodoProvider.tsx is now addressed: you use a shared FILTERS object and consistently reference its values instead of raw strings, which aligns with [CHECKLIST ITEM #10]. The app correctly shows only the new-todo form when there are no todos, uses functional components with hooks, and applies classnames for conditional classes as required.

If you want to polish further, you could ensure the FILTERS values match the exact lowercase pattern from the checklist (e.g. all, active, completed), but this is stylistic and not a blocker for functionality or requirements. Overall, your implementation is coherent, readable, and meets the task standards—well done on addressing the earlier review feedback effectively.

✨ 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-07-05T13:21:17Z

+import React, { useEffect, useState } from 'react';
+import { Filter, FILTERS } from '../types/Filter';
+import { Todo } from '../types/Todo';
+import { TodoContext } from './TodoContext';


This violates checklist item #10: Do not rely on the unknown string, make constants for this. The required pattern uses lowercase string values ('all', 'active', 'completed'), but here the constant values are capitalized ('All', 'Active', 'Completed'). Update these to the lowercase forms and keep using them wherever you set/compare filter.

Implement Todo App features

42809fa

mateacademy-ai-mentor suggested changes Jul 4, 2026

View reviewed changes

Implement todo app with filters

8f7f482

eg3844540-svg requested a review from mateacademy-ai-mentor July 5, 2026 13:19

mateacademy-ai-mentor approved these changes Jul 5, 2026

View reviewed changes

		@@ -0,0 +1,35 @@
		import React, { useEffect, useState } from 'react';

		@@ -0,0 +1 @@
		export type Filter = 'All' \| 'Active' \| 'Completed';

Uh oh!

Conversation

eg3844540-svg commented Jul 4, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants