feat(todo): Notes/Todo split — final integration pass (C5)#2033
Conversation
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? |
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughChangesThe PR separates the Notes and Todo desktop experiences. Notes now supports notes only, while Todo has its own UI, tests, registry import, validated reorder persistence, and CSRF-protected routing. A design document describes the broader storage, API, migration, and collaboration split. Notes and Todo separation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TodoApp
participant TodoDetailPane
participant TodoAPI
participant TodoStore
User->>TodoApp: select list or create list
TodoApp->>TodoAPI: request lists
TodoAPI-->>TodoApp: return lists
TodoApp->>TodoDetailPane: open selected list
TodoDetailPane->>TodoAPI: request or mutate items
TodoAPI->>TodoStore: validate and persist changes
TodoStore-->>TodoAPI: return updated state
TodoAPI-->>TodoDetailPane: return item response
TodoDetailPane-->>User: render updated tasks
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| await self._db.execute("DELETE FROM todo_items WHERE id = ?", (item_id,)) | ||
| await self._db.commit() | ||
|
|
||
| async def reorder_items(self, list_id: str, items: list[dict]) -> None: |
There was a problem hiding this comment.
WARNING: reorder_items applies each UPDATE in a separate statement with no transaction and no validation.
The design doc for #1923 explicitly promises that "partial or invalid payloads (missing items, duplicate positions, unknown ids) are rejected with 400 Bad Request and make no changes." Here a payload that omits items, duplicates positions, or contains unknown ids is applied silently (and non-atomically — an error mid-loop leaves the list partially reordered). Consider wrapping the loop in a single transaction (BEGIN/COMMIT) and validating that every item id belongs to list_id and that the position set is complete and unique before applying.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if err: | ||
| return err | ||
|
|
||
| items = [{"id": e.id, "position": e.position} for e in body.items] |
There was a problem hiding this comment.
SUGGESTION: The reorder payload is forwarded straight to the store without verifying it represents the full, current item set.
If the client sends a partial/buggy list (or an attacker omits items from another list that happen to exist), items can be left with stale position values and the AND list_id = ? guard only makes cross-list ids a silent no-op. It would be safer to cross-check body.items against the list's actual items (all ids present, exactly once, contiguous positions) and return 400 otherwise — matching the contract described in the design doc.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if (!newText.trim() || !doc || adding) return; | ||
| setAdding(true); | ||
| try { | ||
| const r = await fetch(`/api/todo/${doc.id}/items`, { |
There was a problem hiding this comment.
SUGGESTION: The mutating requests in this file (POST/PUT/PATCH/DELETE to /api/todo) use raw fetch instead of the withCsrf helper (desktop/src/lib/csrf.ts) or taos-fetch wrapper.
Per taos-fetch.ts, new code is expected to use withCsrf from day one (it attaches the X-CSRF-Token header and reports backend version). This mirrors the existing NotesApp.tsx pattern, so it is not a new regression, but for truly new code it is cleaner and more robust to route mutations through withCsrf/createTaosFetch so you don't have to hand-maintain the CSRF header and you get version-change detection for free.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No New Issues Found | Recommendation: Merge All previously flagged findings have been verified resolved in the current HEAD ( Resolved (from prior review)
Overview
Files Reviewed (this pass)
Previous Review Summaries (6 snapshots, latest commit 590f5bf)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 590f5bf)Status: No New Issues Found | Recommendation: Merge All previously flagged findings have been resolved in the current HEAD ( Resolved (from prior review)
Overview
Files Reviewed (this pass)
Previous review (commit 27d90f5)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous review (commit 1432fc7)Status: No Issues Found | Recommendation: Merge Incremental review since Incremental changes verified
Files Reviewed (3 incremental files)
Previous review (commit de07336)Status: No Issues Found | Recommendation: Merge All three prior findings are resolved by the incremental commits since
The incremental diff (2 files, 10 lines) introduces no new issues. Files Reviewed (2 incremental files)
Previous review (commit bbb4976)Status: No Issues Found | Recommendation: Merge All three prior findings from the previous review are resolved by commits added since
No new issues introduced in the incremental diff. Files Reviewed (11 files)
Previous review (commit 61e6b28)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (13 files)
Reviewed by hy3:free · Input: 94.7K · Output: 5.7K · Cached: 872.5K |
39ff419 to
bbb4976
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.tsx`:
- Around line 303-320: Clear the stale error state in the successful path of
loadDoc after the latest request is confirmed current, and apply the same reset
to successful addItem, deleteItem, toggleDone, and moveItem mutation paths.
Preserve existing stale-request guards and failure messages while ensuring any
successful operation removes the error banner.
In `@tinyagentos/routes/notes.py`:
- Around line 1-9: Remove the kind field from CreateDocIn and stop passing
body.kind to store.create_doc in the notes route; ensure note creation uses only
the remaining request fields and no longer exposes kind through this API.
🪄 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: 3f966cce-79ed-435f-b678-c75cbe61b535
📒 Files selected for processing (12)
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.tsdocs/design/notes-todo-split.mdtinyagentos/notes/shared_docs_store.pytinyagentos/routes/__init__.pytinyagentos/routes/notes.pytinyagentos/routes/todo.pytinyagentos/todo/todo_store.py
- TodoApp.tsx: clear error state on success in loadDoc, addItem, deleteItem, toggleDone, and moveItem so error banner doesn't persist after recovery - notes.py: remove kind field from CreateDocIn model, hardcode note kind in create_doc since the notes API only creates note-type docs - Remove obsolete test_invalid_kind_returns_400 (kind no longer API-visible) CodeRabbit findings on PR jaylfc#2033
|
|
||
| # App config | ||
| mock_config = MagicMock() | ||
| mock_config.github_app_id = "123456" |
There was a problem hiding this comment.
WARNING: mock_config sets github_app_id but not github_app_private_key.
tinyagentos/routes/github.py:91 requires BOTH cfg.github_app_id AND cfg.github_app_private_key to be truthy before _get_app_installation_token ever calls get_installation_token. With github_app_private_key unset, the App-token path short-circuits and returns None immediately — so get_installation_token is never reached.
Consequences for these new tests:
test_repo_endpoint_still_uses_app_tokenwill FAIL:_get_tokenreturnsNone(App path skipped,ghCLI raisesFileNotFoundError), so/api/github/repo/bob/myreporeturns 401 instead of 200.- The three
assert_not_called()tests (test_starred_uses_gh_cli...,test_notifications_use_gh_cli...,test_auth_status_false_when_only_app_token_available) pass trivially — they never actually exercise the App-token mint path, so they do not verify that user-scoped endpoints skip the App token. The central assertion is vacuous.
Fix: set mock_config.github_app_private_key = "fake-private-key" (the secrets key is already mocked for github-app-private-key), so the App-token path is genuinely reached. Then test_repo_endpoint_still_uses_app_token will exercise the patched get_installation_token, and the assert_not_called() tests will meaningfully assert the App token is skipped for user-scoped endpoints.
| mock_config.github_app_id = "123456" | |
| mock_config.github_app_id = "123456" | |
| mock_config.github_app_private_key = "fake-private-key" |
- TodoApp.tsx: clear error state on success in loadDoc, addItem, deleteItem, toggleDone, and moveItem so error banner doesn't persist after recovery - notes.py: remove kind field from CreateDocIn model, hardcode note kind in create_doc since the notes API only creates note-type docs - Remove obsolete test_invalid_kind_returns_400 (kind no longer API-visible) CodeRabbit findings on PR jaylfc#2033
27d90f5 to
590f5bf
Compare
…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)
- 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)
Remove the last 'note | list' union types from NotesApp, SharedDocsStore, and notes routes. All docs are now 'note' only — todo lists live in their own dedicated TodoStore at /api/todo. - NotesApp.tsx: kind: 'note' (was 'note' | 'list') - NotesApp.test.tsx: drop 'renders only notes, filtering out lists' test - shared_docs_store.py: update docstring from 'notes and lists' to 'notes' - routes/notes.py: update docstring, add cross-ref to /api/todo - docs/design/notes-todo-split.md: add design document for jaylfc#1923
- Wrap all mutating fetch calls (POST/PATCH/DELETE/PUT) with withCsrf() so the X-CSRF-Token header is sent now that todo_router has CSRF protection - Wrap reorder_items in a BEGIN/COMMIT transaction with rollback - Validate reorder payload: all items must belong to target list, positions must be unique — rejects partial/invalid payloads with ValueError before any writes
Docs-Reviewed: Todo app already listed in README.md bundled apps section
- TodoApp.tsx: clear error state on success in loadDoc, addItem, deleteItem, toggleDone, and moveItem so error banner doesn't persist after recovery - notes.py: remove kind field from CreateDocIn model, hardcode note kind in create_doc since the notes API only creates note-type docs - Remove obsolete test_invalid_kind_returns_400 (kind no longer API-visible) CodeRabbit findings on PR jaylfc#2033
590f5bf to
7c2daee
Compare
1 participant
Closes #1923.
What this does
Completes the Notes/Todo split with the final integration pass:
notes(launchpadOrder 16.8) andtodo(16.85) registeredC5 checklist
Commits
ad96c709feat(todo): TodoStore + /api/todo routesb929f6f2feat(todo): TodoApp componenta6260f01fix(todo): Kilo review fixes4002cf77fix(todo): reorder + validatee26bade0fix(todo): duplicate submissions, toggle serialization90141ac8fix(todo): CSRF dependency on todo_routerSummary by CodeRabbit
New Features
Bug Fixes