Skip to content

Comments

initial commit#1272

Open
Wiolip wants to merge 2 commits intomate-academy:masterfrom
Wiolip:develop
Open

initial commit#1272
Wiolip wants to merge 2 commits intomate-academy:masterfrom
Wiolip:develop

Conversation

@Wiolip
Copy link

@Wiolip Wiolip commented Feb 15, 2026

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 requesting changes because there is at least one functional bug plus several explicit checklist violations that need to be addressed before this can be considered complete.

Critical/blocking issues:

  1. In src/components/TodoHeader.tsx, the focus handler is set as setFocusHandler(() => focusInput), which stores a function that merely returns focusInput instead of calling it. This breaks the required focusing behavior when the handler is invoked. Update it to either setFocusHandler(focusInput) or setFocusHandler(() => focusInput()) so calling the handler actually focuses the input.
  2. Inline editing in src/components/TodoItem.tsx does not reliably cancel on Escape: handleKeyUp sets isEditing to false, but onBlur still calls handleSubmit, which can re-save the value after Escape is pressed. You need to prevent onBlur from saving when Escape was used (for example, track a canceled flag/ref set on Escape and check it in the blur handler) so Escape truly cancels editing.

Checklist-required fixes (also blocking for this task):

  • Filtering currently relies on raw string literals ('all' | 'active' | 'completed') in both src/context/TodoContext.tsx and src/components/TodoFilter.tsx. Introduce a shared FILTERS constant and use it for the filter type, initial state, comparisons, and mapping/filter links to satisfy the "no magic strings" requirement [CHECKLIST ITEM #10].
  • Several components (TodoFilter, TodoHeader, TodoItem) build conditional class names with template strings; the task requires using the classnames library for calculated classes [CHECKLIST ITEM #7]. Replace those template literals with classNames(...) calls.
  • Callbacks that modify main state (addTodo in TodoHeader, updateTodo in TodoItem) are called with raw input; per the task, they should receive already-prepared data [CHECKLIST ITEM #2]. Trim the values in the component (e.g. const trimmed = value.trim(); if (!trimmed) handle delete or skip; else updateTodo(..., trimmed);) before passing them to context.

You’ve implemented most of the required behavior (localStorage, filtering, toggle all, clear completed, inline editing) well; once these functional and checklist-required issues are fixed, your solution will be in good shape.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

</span>

<nav className="filter" data-cy="Filter">
{(['all', 'active', 'completed'] as const).map(type => (

Choose a reason for hiding this comment

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

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

const FILTERS = {
  all: 'all',
  completed: 'completed',
  active: 'active',
};

You currently map over the inline array ['all', 'active', 'completed'] and use raw 'all' in the href. Define and import a shared FILTERS constant (or export it from the context) and use it here (e.g., Object.values(FILTERS) or an ordered array exported alongside FILTERS) so the component does not rely on duplicated magic strings.

<a
key={type}
href={`#/${type === 'all' ? '' : type}`}
className={`filter__link ${filter === type ? 'selected' : ''}`}

Choose a reason for hiding this comment

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

This violates checklist item #7: "Use classnames lib for calculated classes." The className on the link is built with a template string: className={`filter__link ${filter === type ? 'selected' : ''}`}. Consider importing classnames and using classNames('filter__link', { selected: filter === type }) to follow the checklist and make class logic clearer and less error-prone.

Comment on lines 12 to 13
setFocusHandler(() => focusInput);
focusInput();

Choose a reason for hiding this comment

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

This will not set the focus handler correctly. setFocusHandler(() => focusInput) stores a function that returns focusInput instead of storing a function that performs focusing. As a result focusHandler() (called elsewhere) will return the focus function instead of calling it — the input won't be focused. Use either setFocusHandler(focusInput) or setFocusHandler(() => focusInput()) so the stored handler actually runs the focus action when invoked.

Comment on lines 16 to 20
const handleAddTodo = (e: React.FormEvent) => {
e.preventDefault();
if (query.trim()) {
addTodo(query);
setQuery('');

Choose a reason for hiding this comment

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

This violates checklist item #2: "Callbacks that work with the main state should take prepared data instead of the whole child's state." You're calling addTodo(query) with the raw input. Trim the input before calling the context callback (e.g. const trimmed = query.trim(); if (trimmed) addTodo(trimmed);) so the callback receives prepared data.

{todos.length > 0 && (
<button
type="button"
className={`todoapp__toggle-all ${allCompleted ? 'active' : ''}`}

Choose a reason for hiding this comment

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

This violates Basic React Checklist item #7: "Use classnames lib for calculated classes." The toggle-all button computes its class with a template literal (className={todoapp__toggle-all ${allCompleted ? 'active' : ''}}) — switch to the classnames library to match the checklist (e.g. classNames('todoapp__toggle-all', { active: allCompleted })).


return (
<div
className={`todo ${todo.completed ? 'completed' : ''} ${isEditing ? 'editing' : ''}`}

Choose a reason for hiding this comment

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

This uses a template string to build the element class. The checklist expects the classnames library for calculated classes (Basic React Checklist item #7). Replace this with classNames('todo', { completed: todo.completed, editing: isEditing }) (import classnames as needed).

Comment on lines 16 to 18
const handleSubmit = () => {
updateTodo(todo.id, tempTitle);
setIsEditing(false);

Choose a reason for hiding this comment

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

handleSubmit forwards the raw tempTitle to updateTodo. The Task Checklist requires callbacks that work with main state to take prepared data instead of the child's raw state. Trim the value before calling updateTodo (e.g. const trimmed = tempTitle.trim(); updateTodo(todo.id, trimmed);) so the context receives prepared data.

Comment on lines 21 to 56
const handleKeyUp = (e: React.KeyboardEvent) => {
if (e.key === 'Escape') {
setTempTitle(todo.title);
setIsEditing(false);
}

if (e.key === 'Enter') {
handleSubmit();
}
};

return (
<div
className={`todo ${todo.completed ? 'completed' : ''} ${isEditing ? 'editing' : ''}`}
data-cy="Todo"
>
{/* eslint-disable-next-line jsx-a11y/label-has-associated-control */}
<label className="todo__status-label">
<input
type="checkbox"
className="todo__status"
checked={todo.completed}
onChange={() => toggleTodo(todo.id)}
data-cy="TodoStatus"
/>
</label>
{isEditing ? (
<input
ref={editInputRef}
type="text"
className="todo__title-field"
style={{ width: '100%', boxSizing: 'border-box' }}
value={tempTitle}
onChange={e => setTempTitle(e.target.value)}
onBlur={handleSubmit}
onKeyUp={handleKeyUp}

Choose a reason for hiding this comment

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

Pressing Escape should cancel editing without saving. Currently handleKeyUp resets tempTitle and sets isEditing to false, but the input's onBlur (line 55) may still fire and call handleSubmit, causing a save. Consider preventing onBlur from saving when Escape was pressed (for example, track a canceledRef boolean that is set on Escape and checked in the onBlur handler, or call event.preventDefault() flow accordingly) so Escape truly cancels editing per the requirement.

completed: boolean;
}

type FilterType = 'all' | 'active' | 'completed';

Choose a reason for hiding this comment

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

This file uses raw filter string literals instead of a shared constant. This violates checklist item #10: "Do not rely on the unknown string, make constants for this:

const FILTERS = {
  all: ‘all’,
  completed: ‘completed’,
  active: ‘active’,
};

Declare a FILTERS constant (export it if needed) and use it for the FilterType definition and for initializing filter state so the code doesn't rely on magic strings.

Comment on lines 56 to 66
const visibleTodos = useMemo(() => {
return todos.filter(todo => {
if (filter === 'active') {
return !todo.completed;
}

if (filter === 'completed') {
return todo.completed;
}

return true;

Choose a reason for hiding this comment

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

The visibleTodos logic compares filter to string literals ('active' / 'completed'). Replace these checks with references to the FILTERS constant (e.g. filter === FILTERS.active) so the filtering logic uses the defined constants, matching checklist item #10.

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

Your solution is approved because it correctly implements all the required Todo app features and there are no critical blockers. You use React Context to manage todos, persist them to localStorage with JSON.stringify on every change, and implement filtering, toggle-all, clear-completed, and inline editing (including trim, delete-on-empty, Enter/Blur save, and Escape cancel) exactly as specified. The UI logic matches the requirements: when there are no todos only the new-todo input is shown, the active items count is displayed, and the clear-completed button is disabled when there are no completed items.

Two non-blocking improvements you might consider: add a safe try/catch around JSON.parse when reading from localStorage so corrupted data doesn’t break initialization, and improve accessibility by adding aria-label or title attributes to controls like the toggle-all and delete buttons. You could also polish the footer text to handle singular vs plural (e.g., “1 item left” vs “2 items left”). Overall, this is a clean, well-structured implementation with thoughtful handling of edge cases—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

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