From 79b3097410c512d323c978fc4fb55d5313709303 Mon Sep 17 00:00:00 2001
From: Kyle Pope <kyle.a.pope@outlook.com>
Date: Mon, 23 Feb 2026 20:38:01 +0800
Subject: [PATCH] Fix QA review issues: transaction safety, validation,
 accessibility

Critical fixes:
- [C1] _reactivate_recurring_todos now uses flush + with_for_update
  instead of mid-request commit; get_todos commits the full transaction
- [C2] recurrence_rule validated via Literal["daily","weekly","monthly"]
  in Pydantic schemas (rejects invalid values with 422)

Warnings fixed:
- [W3] Clear due_time when due_date is set to null in update endpoint

Suggestions applied:
- [S2] Wrap filteredTodos in useMemo for consistent memoization
- [S6] Add aria-labels to edit/delete icon buttons

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---
 backend/app/routers/todos.py                | 16 ++++++++++++---
 backend/app/schemas/todo.py                 |  5 +++--
 frontend/src/components/todos/TodoItem.tsx  |  3 ++-
 frontend/src/components/todos/TodosPage.tsx | 22 ++++++++++++---------
 4 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/backend/app/routers/todos.py b/backend/app/routers/todos.py
index 1235d49..840669e 100644
--- a/backend/app/routers/todos.py
+++ b/backend/app/routers/todos.py
@@ -73,7 +73,11 @@ def _calculate_recurrence(
 
 
 async def _reactivate_recurring_todos(db: AsyncSession) -> None:
-    """Auto-reactivate recurring todos whose reset_at has passed."""
+    """Auto-reactivate recurring todos whose reset_at has passed.
+
+    Uses flush (not commit) so changes are visible to the subsequent query
+    within the same transaction. The caller's commit handles persistence.
+    """
     now = datetime.now()
     query = select(Todo).where(
         and_(
@@ -82,7 +86,7 @@ async def _reactivate_recurring_todos(db: AsyncSession) -> None:
             Todo.reset_at.isnot(None),
             Todo.reset_at <= now,
         )
-    )
+    ).with_for_update()
     result = await db.execute(query)
     todos = result.scalars().all()
 
@@ -95,7 +99,7 @@ async def _reactivate_recurring_todos(db: AsyncSession) -> None:
         todo.next_due_date = None
 
     if todos:
-        await db.commit()
+        await db.flush()
 
 
 @router.get("/", response_model=List[TodoResponse])
@@ -130,6 +134,8 @@ async def get_todos(
     result = await db.execute(query)
     todos = result.scalars().all()
 
+    await db.commit()
+
     return todos
 
 
@@ -189,6 +195,10 @@ async def update_todo(
             update_data["reset_at"] = None
             update_data["next_due_date"] = None
 
+    # Clear due_time if due_date is being removed
+    if "due_date" in update_data and update_data["due_date"] is None:
+        update_data["due_time"] = None
+
     for key, value in update_data.items():
         setattr(todo, key, value)
 
diff --git a/backend/app/schemas/todo.py b/backend/app/schemas/todo.py
index a36ffb3..80338eb 100644
--- a/backend/app/schemas/todo.py
+++ b/backend/app/schemas/todo.py
@@ -3,6 +3,7 @@ from datetime import datetime, date, time
 from typing import Optional, Literal
 
 TodoPriority = Literal["none", "low", "medium", "high"]
+RecurrenceRule = Literal["daily", "weekly", "monthly"]
 
 
 class TodoCreate(BaseModel):
@@ -12,7 +13,7 @@ class TodoCreate(BaseModel):
     due_date: Optional[date] = None
     due_time: Optional[time] = None
     category: Optional[str] = None
-    recurrence_rule: Optional[str] = None
+    recurrence_rule: Optional[RecurrenceRule] = None
     project_id: Optional[int] = None
 
 
@@ -24,7 +25,7 @@ class TodoUpdate(BaseModel):
     due_time: Optional[time] = None
     completed: Optional[bool] = None
     category: Optional[str] = None
-    recurrence_rule: Optional[str] = None
+    recurrence_rule: Optional[RecurrenceRule] = None
     project_id: Optional[int] = None
 
 
diff --git a/frontend/src/components/todos/TodoItem.tsx b/frontend/src/components/todos/TodoItem.tsx
index b65ae34..511a7e5 100644
--- a/frontend/src/components/todos/TodoItem.tsx
+++ b/frontend/src/components/todos/TodoItem.tsx
@@ -149,12 +149,13 @@ export default function TodoItem({ todo, onEdit }: TodoItemProps) {
       )}
 
       {/* Actions */}
-      <Button variant="ghost" size="icon" onClick={() => onEdit(todo)} className="h-7 w-7 shrink-0">
+      <Button variant="ghost" size="icon" onClick={() => onEdit(todo)} className="h-7 w-7 shrink-0" aria-label="Edit todo">
         <Pencil className="h-3 w-3" />
       </Button>
       <Button
         variant="ghost"
         size="icon"
+        aria-label="Delete todo"
         onClick={() => deleteMutation.mutate()}
         disabled={deleteMutation.isPending}
         className="h-7 w-7 shrink-0 hover:bg-destructive/10 hover:text-destructive"
diff --git a/frontend/src/components/todos/TodosPage.tsx b/frontend/src/components/todos/TodosPage.tsx
index 1a47388..f67189f 100644
--- a/frontend/src/components/todos/TodosPage.tsx
+++ b/frontend/src/components/todos/TodosPage.tsx
@@ -45,15 +45,19 @@ export default function TodosPage() {
     return Array.from(cats).sort();
   }, [todos]);
 
-  const filteredTodos = todos.filter((todo) => {
-    if (filters.priority && todo.priority !== filters.priority) return false;
-    if (filters.category && todo.category?.toLowerCase() !== filters.category.toLowerCase())
-      return false;
-    if (!filters.showCompleted && todo.completed) return false;
-    if (filters.search && !todo.title.toLowerCase().includes(filters.search.toLowerCase()))
-      return false;
-    return true;
-  });
+  const filteredTodos = useMemo(
+    () =>
+      todos.filter((todo) => {
+        if (filters.priority && todo.priority !== filters.priority) return false;
+        if (filters.category && todo.category?.toLowerCase() !== filters.category.toLowerCase())
+          return false;
+        if (!filters.showCompleted && todo.completed) return false;
+        if (filters.search && !todo.title.toLowerCase().includes(filters.search.toLowerCase()))
+          return false;
+        return true;
+      }),
+    [todos, filters]
+  );
 
   const now = new Date();
   const todayStr = `${now.getFullYear()}-${String(now.getMonth() + 1).padStart(2, '0')}-${String(now.getDate()).padStart(2, '0')}`;