feat(todo): add dedicated TodoApp component with checklist UI, reorder, due dates#1946
feat(todo): add dedicated TodoApp component with checklist UI, reorder, due dates#1946hognek wants to merge 4 commits into
Conversation
…r, due dates
- New TodoApp.tsx: checklist UI with checkboxes, optimistic toggle,
move-up/down reorder buttons, due date display with overdue
highlighting, inline add/edit, Pico CSS responsive layout
- Hits /api/todo endpoints (C1 backend) instead of shared /api/notes
- Splits incomplete/complete items into separate sections
- Move-up/down reorder sends PUT /api/todo/{id}/items/reorder
- Optimistic toggle for complete with revert on failure
- Defensive against missing items array in API responses
- Update app-registry: todo now lazy-loads from TodoApp.tsx directly
- Refactor NotesApp.tsx: remove TODO_CONFIG and TodoApp export,
keep Notes-only config
- Update NotesApp tests: remove TodoApp import and Todo-specific tests
- Add TodoApp.test.tsx: 12 Vitest tests covering lists, items,
toggle, delete, reorder, due dates, overdue, inline edit
Part of jaylfc#1923 (C2 — frontend split)
📝 WalkthroughWalkthroughChangesTodo application extraction
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TodoApp
participant TodoDetailPane
participant TodoAPI
User->>TodoApp: Select or create todo list
TodoApp->>TodoAPI: Load or create list
TodoApp->>TodoDetailPane: Render selected list
TodoDetailPane->>TodoAPI: Load list items
User->>TodoDetailPane: Modify item
TodoDetailPane->>TodoAPI: Persist item change
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ); | ||
| } | ||
|
|
||
| if (error || !doc) { |
There was a problem hiding this comment.
WARNING: A transient action error (failed toggle/delete/edit/reorder) sets the shared error state, which this early-return then uses to replace the ENTIRE detail pane with just the error text. The user loses the whole task list after a single failed toggle.
Use a separate, non-fatal actionError/banner state for transient action failures (like the banner already rendered at line 493) instead of the load-error error that controls this guard. This error should only represent "list could not be loaded".
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| <button | ||
| type="button" | ||
| onClick={() => onMove(item.id, 1)} | ||
| disabled={item.position >= itemCount - 1} |
There was a problem hiding this comment.
WARNING: Move up/down uses item.position against the FULL list (itemCount = items.length), but incomplete and complete items are rendered in two separate <ul> sections. If the adjacent item by position is in the other section, moveItem swaps it within the full array (line 424) yet the UI recomputes the two sections, so the moved task appears to NOT move. Likewise the disabled check (position >= itemCount - 1) can wrongly enable/disable move buttons across the incomplete/complete boundary.
Recompute reorder indices within the visible section only, or operate on a single flat list in the UI rather than splitting by done.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| body: JSON.stringify({ title: title.trim() }), | ||
| }); | ||
| if (!r.ok) throw new Error("Could not create list."); | ||
| const doc: TodoList = await r.json(); |
There was a problem hiding this comment.
SUGGESTION: On success you call onCreated(doc) and hide the form via setShowCreate(false), but creating is never reset to false. It only resets in the catch. If the component is ever reused/re-opened while mounted, it would stay in the creating state.
| const doc: TodoList = await r.json(); | |
| const doc: TodoList = await r.json(); | |
| onCreated(doc); | |
| setCreating(false); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| try { | ||
| const r = await fetch(`/api/todo/${listId}`); | ||
| if (!r.ok) throw new Error("Could not load list."); | ||
| const data: TodoDetail = await r.json(); |
There was a problem hiding this comment.
SUGGESTION: The PR claims to be "defensive against missing items array," but r.json() is cast straight to TodoDetail and doc.items is only guarded later at line 468. If the response omits items or returns a non-array shape, incomplete/complete filters (line 469-470) run on undefined and throw. Validate items here before setDoc, e.g. coerce items: Array.isArray(data.items) ? data.items : [] when building the typed value.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No New Issues Found | Recommendation: Merge Overview
Incremental Changes (commit f704cc8..3d2625d)
Prior Findings Resolved (verified this run)
Files Reviewed (incremental: 2 changed)
Files unchanged since previous review (NotesApp.tsx, app-registry.ts, tests/NotesApp.test.tsx) were not re-analyzed. Previous Review Summaries (3 snapshots, latest commit f704cc8)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit f704cc8)Status: No New Issues Found | Recommendation: Merge Overview
Prior Findings Resolved (incremental)
Files Reviewed (incremental: 1 changed)
Files unchanged since previous review (NotesApp.tsx, app-registry.ts, tests) were not re-analyzed. Previous review (commit 5659267)Status: No New Issues Found | Recommendation: Merge Overview
Prior Findings Resolved (incremental)
Files Reviewed (incremental: 1 changed)
Files unchanged since previous review (NotesApp.tsx, app-registry.ts, tests) were not re-analyzed. Previous review (commit 3c31cb6)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files)
Reviewed by hy3:free · Input: 36.8K · Output: 1.7K · Cached: 100K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
desktop/src/apps/TodoApp.test.tsx (2)
299-325: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise both optimistic rendering and failure rollback.
The PATCH resolves immediately and the test only inspects its payload, so it does not verify that completion appears before the response or that a failed request restores the original state. Add deferred-success and failure cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/apps/TodoApp.test.tsx` around lines 299 - 325, The TodoApp PATCH test only checks the request payload and must cover UI state transitions. Update the test around the existing toggle flow to use a deferred PATCH response, assert the task appears completed before resolving it, then add a failed-PATCH case that rejects the request and verifies the task returns to its original incomplete state.
404-431: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact reordered sequence.
These assertions pass for any array, including unchanged or invalid positions. Verify the expected item IDs and positions—and ideally the resulting visible order—to cover the reordering contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/apps/TodoApp.test.tsx` around lines 404 - 431, Strengthen the reorder test around the “Move task down” interaction by asserting the PUT request’s items contain the expected task IDs in the reordered sequence with their correct positions, rather than only checking that items is an array. Also verify the rendered task order after the move so the test covers both the API payload and visible reordering behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@desktop/src/apps/TodoApp.test.tsx`:
- Around line 280-295: Update the test “splits incomplete and completed items
into sections” to assert the actual completed fixture, “Already done,” within
the completed section, and verify that the incomplete fixture remains within the
incomplete section. Remove the incorrect “Pick up laundry” assertion while
preserving the existing section-label checks.
In `@desktop/src/apps/TodoApp.tsx`:
- Around line 322-330: Guard the submission handlers addItem and the
corresponding list-creation handlers near the referenced sections against
in-flight requests by returning early when their existing loading flags are
active. Keep the current input validation and POST behavior unchanged, and
ensure repeated Enter presses cannot submit duplicate tasks or lists while a
request is pending.
- Around line 378-410: Update toggleDone to serialize PATCH requests per itemId,
preventing overlapping completion updates when the checkbox is toggled rapidly.
Disable the affected item’s checkbox while its request is pending or queue
subsequent desired states, and ensure rollback restores the correct prior state
for the serialized update rather than blindly using !done.
- Around line 716-727: Update loadLists to throw when the fetch response is not
OK, and track the resulting load failure separately from the lists data. Render
the load error state instead of “No lists yet” when loading fails, while
preserving the existing successfully loaded lists behavior.
- Around line 449-501: Restructure the TodoApp detail render around the existing
header and mobile Back button so they remain available whenever a document is
loaded, including mutation-error states. Reserve the fatal early-return state
for !doc, keep loading and error content within the loaded layout as
appropriate, and preserve the existing inline error banner so mutation errors do
not replace the document content.
- Around line 348-351: Update the TodoApp item deletion and reorder logic around
the setDoc updater and movement handlers to operate within the currently
rendered incomplete or completed section, using rendered indices rather than the
combined items array or global position values. Prevent moves across section
boundaries, derive button enabled states from section-local boundaries, and
normalize remaining positions after deletion so no gaps can make controls no-op.
---
Nitpick comments:
In `@desktop/src/apps/TodoApp.test.tsx`:
- Around line 299-325: The TodoApp PATCH test only checks the request payload
and must cover UI state transitions. Update the test around the existing toggle
flow to use a deferred PATCH response, assert the task appears completed before
resolving it, then add a failed-PATCH case that rejects the request and verifies
the task returns to its original incomplete state.
- Around line 404-431: Strengthen the reorder test around the “Move task down”
interaction by asserting the PUT request’s items contain the expected task IDs
in the reordered sequence with their correct positions, rather than only
checking that items is an array. Also verify the rendered task order after the
move so the test covers both the API payload and visible reordering behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ff5a78f-647a-4345-b6d2-5ca09fd69132
📒 Files selected for processing (6)
desktop/src/apps/NotesApp.test.tsxdesktop/src/apps/NotesApp.tsxdesktop/src/apps/TodoApp.test.tsxdesktop/src/apps/TodoApp.tsxdesktop/src/apps/__tests__/NotesApp.test.tsxdesktop/src/registry/app-registry.ts
💤 Files with no reviewable changes (1)
- desktop/src/apps/NotesApp.tsx
| async function toggleDone(itemId: string, done: boolean) { | ||
| if (!doc) return; | ||
| // Optimistic update | ||
| setDoc((prev) => | ||
| prev | ||
| ? { | ||
| ...prev, | ||
| items: prev.items.map((i) => | ||
| i.id === itemId ? { ...i, done } : i, | ||
| ), | ||
| } | ||
| : prev, | ||
| ); | ||
| try { | ||
| const r = await fetch(`/api/todo/${doc.id}/items/${itemId}`, { | ||
| method: "PATCH", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ done }), | ||
| }); | ||
| if (!r.ok) throw new Error("Could not update task."); | ||
| } catch (e) { | ||
| // Revert on failure | ||
| setDoc((prev) => | ||
| prev | ||
| ? { | ||
| ...prev, | ||
| items: prev.items.map((i) => | ||
| i.id === itemId ? { ...i, done: !done } : i, | ||
| ), | ||
| } | ||
| : prev, | ||
| ); | ||
| setError(e instanceof Error ? e.message : "Could not update task."); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Serialize completion updates per item.
The checkbox remains enabled while PATCH is pending. Rapid toggles can resolve out of order, leaving the server value different from the UI; the !done rollback also becomes incorrect with overlapping requests. Disable or queue updates per item.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@desktop/src/apps/TodoApp.tsx` around lines 378 - 410, Update toggleDone to
serialize PATCH requests per itemId, preventing overlapping completion updates
when the checkbox is toggled rapidly. Disable the affected item’s checkbox while
its request is pending or queue subsequent desired states, and ensure rollback
restores the correct prior state for the serialized update rather than blindly
using !done.
- Change error guard from (error || !doc) to (!doc) so transient action errors show inline banner instead of blanking the pane - Reset creating=false on success in CreateTodoForm - Move-up/down uses full-list positions correctly; items share a single position space regardless of visual section split
- Kilo WARNING: reorder items within their visible section (incomplete/complete) instead of the full list, fixing cross-boundary swap bug and incorrect move-up/move-down disabled checks - Kilo SUGGESTION: validate doc.items as array before setDoc in loadDoc to guard against missing/non-array items from the API Desktop tests: 12/12 pass. TypeScript: clean.
…ror handling - Guard addItem/create against duplicate Enter submissions while request pending - Serialize toggleDone PATCH per itemId to prevent overlapping completion updates - Show load error instead of 'No lists yet' when list fetch fails - Keep mobile back-navigation available during detail load errors - Fix test: assert actual completed fixture (Already done, not Pick up laundry)
|
Code is clean and every bot finding is folded (only 2 trivial test nitpicks left), nice. Held purely on merge order: the new TodoApp targets |
2 participants
Task: C2 of #1923 (Notes/Todo split). Builds on C1 backend (TodoStore + /api/todo routes).
Changes
New:
desktop/src/apps/TodoApp.tsx(845 lines)Modified:
desktop/src/registry/app-registry.ts@/apps/TodoAppinstead of@/apps/NotesAppRefactored:
desktop/src/apps/NotesApp.tsxTests
TodoApp.test.tsx: 12 tests (list CRUD, items, toggle, delete, add, reorder, due dates, overdue, inline edit)NotesApp.test.tsx: removed TodoApp import and Todo-specific tests (now 5 Notes-only tests)__tests__/NotesApp.test.tsx: removed TodoApp section (now 5 Notes-only tests)Test results
Bot-fix (commit 3d2625d)
Depends on: C1 backend (#1923 todo-store branch — feat/1923-todo-store)