docs(design): split Notes/Todo into separate apps (#1923)#1935
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: 51 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 (1)
📝 WalkthroughWalkthroughThe design note specifies separate Notes and Todo stores, APIs, agent tools, frontend app trees, registry entries, migration phases, and implementation tasks. ChangesNotes and Todo split
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
🚥 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 |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| ### 2c. Agent tools | ||
|
|
||
| | Tool | Current | After | |
There was a problem hiding this comment.
SUGGESTION: The §2c tool-mapping table header/columns are inconsistent.
Header says Tool | Current | After, but rows 1–2 place the new name in the "Current" column (e.g. notes_list_shared_docs | → notes_list_docs) while row 3 places the change in the "After" column (→ removed from notes). As written the columns read ambiguously — the first two rows are effectively swapped compared to the header's intent. Recommend a clear three-column mapping, e.g. Tool | Current name | New name/status, so each row is unambiguous.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| ### 3b. Target: Two separate component trees | ||
|
|
||
| ``` |
There was a problem hiding this comment.
SUGGESTION: The target component tree lists the same test files in two locations.
NotesApp.test.tsx and TodoApp.test.tsx appear both under desktop/src/apps/ (lines 224, 226) and again under desktop/src/apps/apps/__tests__/ (lines 228–229). This is contradictory — it's unclear whether tests live at the app root or in a nested __tests__/ dir. Pick one convention (the nested apps/__tests__/ path also looks like a typo, duplicating apps/).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| class AddTodoItemIn(BaseModel): | ||
| text: str | ||
| due_at: str | None = None # ISO-8601 timestamp |
There was a problem hiding this comment.
SUGGESTION: ISO-8601 string → REAL timestamp conversion is unspecified.
AddTodoItemIn/PatchTodoItemIn model due_at/remind_at as str | None ISO-8601 (lines 161–162, 167–168), but the todo_items schema stores them as REAL (lines 53–54). The design never states where/how the ISO string is parsed and validated into a REAL epoch, nor what happens on malformed input. Specify the conversion point (Pydantic validator vs router) and the error/validation behavior to avoid silent bad data.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| - The existing entry revision history is preserved as-is — it's useful for | ||
| both apps | ||
|
|
||
| Schema (existing, minus `kind` and `done`): |
There was a problem hiding this comment.
SUGGESTION: Inconsistent description of where revision history lives after the split.
§1c says member/permission/revision logic is extracted into tinyagentos/collaboration/ (line 104), but §1b says NotesStore keeps shared_docs_store.py with the entry revision history "preserved as-is" (lines 78–79) and the schema block comments "Member + revision tables shared via shared_collaboration module" (line 101). It's unclear whether the revision tables are moved out of the notes store into the shared module (requiring refactor + migration of existing revision data) or left in place. Clarify, since moving shared tables impacts the migration plan and existing data.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (1 files)
Incremental Notes
Previous Review Summaries (3 snapshots, latest commit e5cd60f)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit e5cd60f)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 files)
Prior finding resolved by this commit
Previous review (commit 45e100a)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (1 files)
Previously reported issues (now resolved)The following prior findings were addressed by the latest commit and are no longer applicable:
Fix these issues in Kilo Cloud Previous review (commit 92109e0)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (1 files)
Reviewed by hy3:free · Input: 73K · Output: 2.1K · Cached: 140.3K |
|
Two things before merge:
|
|
Addressed review feedback:
Branch: |
|
Docs-only, near-ready. Four consistency nits before merge: DESIGN.md:200 the tool-mapping table puts the NEW name in the Current column (misleads on rename direction); DESIGN.md:221 NotesApp/TodoApp.test.tsx are listed in two tree locations (ambiguous target path); DESIGN.md:161 due_at/remind_at typed str ISO-8601 in the models but todo_items schema stores REAL/timestamp (specify the conversion); DESIGN.md:81 sect 1c says revision logic moves to collaboration/ while 1b says NotesStore keeps it (contradictory). Fix those and I merge. |
Covers storage (separate TodoStore + NotesStore with shared collaboration module), API surface (/api/todo with reorder/due-date endpoints), component split (TodoApp.tsx + refactored NotesApp.tsx), migration plan (additive, kind=list → todo_items with position derived from created_at), and phased rollout via child tasks C1–C4.
…lo consistency nits - Move DESIGN.md from repo root to docs/design/notes-todo-split.md - Fix tool-mapping table header: Current Tool | After (rename) | Purpose - Remove duplicated test file entries from component tree (line 221) - Document ISO-8601 to REAL timestamp conversion for due_at/remind_at - Clarify revision history lives in shared collaboration module, not NotesStore
92109e0 to
45e100a
Compare
|
Addressed review feedback:
PR ready for re-review. |
| **Timestamp conversion:** `due_at` and `remind_at` arrive as ISO-8601 | ||
| strings (e.g. `"2026-07-18T14:00:00Z"`) from the API. The route handler | ||
| converts them to SQLite REAL (Unix epoch seconds) via | ||
| `datetime.fromisoformat()` before passing to the store. The store writes |
There was a problem hiding this comment.
SUGGESTION: datetime.fromisoformat() cannot parse the Z suffix in the documented example string "2026-07-18T14:00:00Z" on Python < 3.11 (it raises ValueError). Since the doc mandates fromisoformat() for exactly that ISO-8601-with-Z format, the design should either require Python >= 3.11 or specify stripping/replacing the trailing Z (e.g. s.replace("Z", "+00:00")) before parsing. Otherwise the conversion will fail at runtime for Z-ending timestamps.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@docs/design/notes-todo-split.md`:
- Around line 110-115: Update the fenced code blocks containing the
collaboration directory tree and frontend component tree in the design notes to
specify the text language, preserving their tree content and formatting while
resolving Markdownlint MD040.
- Around line 260-270: Revise the app-registry guidance to explicitly require
updating the `todo` entry’s lazy import from `@/apps/NotesApp` to
`@/apps/TodoApp`, while leaving the existing `notes` entry unchanged. Remove or
correct the statement that no `app-registry.ts` changes are needed.
- Around line 322-325: Align the `notes_set_done` removal timing across the
Phase 3/C3 plan and the related risk-mitigation sections. Choose a single
cutover point, document whether the existing tool remains registered as a
compatibility path until that point, and ensure all references consistently
describe the same behavior through C4.
- Around line 308-314: Revise Step 6 in the migration plan so legacy shared_docs
rows with kind='list' are first deleted or archived after confirming the split
is stable. Only then remove the kind column or normalize remaining rows to note,
preserving the distinction for retained source data until cleanup is complete.
- Around line 306-314: Update the migration plan around Steps 4–6 to match the
storage design: explicitly state whether collaboration data remains in the
shared database or is copied into TodoStore. If copied, replace the
todo_members-only migration with migration of both member and revision rows,
preserving their IDs and all references, and include the required source/target
table handling.
- Around line 293-304: Update the position calculation in the shared_doc_entries
migration INSERT to use a deterministic ordering by created_at and a stable
secondary key such as id. Ensure the correlated COUNT for each entry counts rows
preceding or equal to it under that same composite order, producing unique
positions while preserving chronological ordering.
- Around line 136-148: Update the Todo API contract table and its surrounding C1
specification to define authorization for every listed route, including
owner/member permissions and archived-list behavior, and require item_id/list_id
scoping validation on item operations and reorder requests. Align these checks
with the membership, permission, archive-state, and entry-ownership checks used
by notes_tools.py, covering reads, writes, membership changes, and reordering.
- Around line 279-300: Update the migration described in
migrate_notes_todo_split.py to be safely rerunnable: wrap each migration phase
in a transaction or durable checkpoint, make table creation non-failing when
tables already exist, and add explicit conflict handling to both todo_lists and
todo_items inserts. Also define how membership data is migrated and checkpointed
so crashes or partial completion can resume without duplicate rows or lost
members.
🪄 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: e6c2f127-737e-4428-9b22-86eaeb7ffecb
📒 Files selected for processing (1)
docs/design/notes-todo-split.md
| ``` | ||
| tinyagentos/collaboration/ | ||
| __init__.py | ||
| member_store.py -- member CRUD, permission check, discuss_channel | ||
| revision_store.py -- immutable revision log with diff/snapshot | ||
| constants.py -- VALID_PERMISSIONS, VALID_ACTIONS |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add languages to the fenced tree blocks.
The collaboration directory tree and frontend component tree use unlabeled fenced code blocks. Mark them as text (or another appropriate language) to resolve Markdownlint MD040.
Also applies to: 231-237
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 110-110: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/design/notes-todo-split.md` around lines 110 - 115, Update the fenced
code blocks containing the collaboration directory tree and frontend component
tree in the design notes to specify the text language, preserving their tree
content and formatting while resolving Markdownlint MD040.
Source: Linters/SAST tools
| | Method | Path | Purpose | | ||
| |--------|------|---------| | ||
| | GET | `/api/todo` | List user's todo lists (non-archived) | | ||
| | POST | `/api/todo` | Create a new todo list | | ||
| | GET | `/api/todo/{list_id}` | Get list with items (ordered by position) | | ||
| | PATCH | `/api/todo/{list_id}` | Rename or archive list | | ||
| | POST | `/api/todo/{list_id}/items` | Add item (appended at end) | | ||
| | PATCH | `/api/todo/{list_id}/items/{item_id}` | Toggle done, set due date, edit text | | ||
| | DELETE | `/api/todo/{list_id}/items/{item_id}` | Delete item | | ||
| | PUT | `/api/todo/{list_id}/items/reorder` | Batch reorder: `{items: [{id, position}, ...]}` | | ||
| | GET | `/api/todo/{list_id}/members` | List members | | ||
| | POST | `/api/todo/{list_id}/members` | Add member (user or agent) | | ||
| | DELETE | `/api/todo/{list_id}/members/{type}/{id}` | Remove member | |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Specify authorization and ID scoping for Todo routes.
The API contract does not define owner/member permissions or require item_id to belong to list_id. Existing tinyagentos/tools/notes_tools.py Lines 81-129 explicitly checks membership, permission, archive state, and entry ownership; C1 should define equivalent checks for every Todo read/write/reorder endpoint.
🤖 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 `@docs/design/notes-todo-split.md` around lines 136 - 148, Update the Todo API
contract table and its surrounding C1 specification to define authorization for
every listed route, including owner/member permissions and archived-list
behavior, and require item_id/list_id scoping validation on item operations and
reorder requests. Align these checks with the membership, permission,
archive-state, and entry-ownership checks used by notes_tools.py, covering
reads, writes, membership changes, and reordering.
| No changes to `app-registry.ts` needed — the two app IDs (`notes`, `todo`) | ||
| already exist and will continue to lazy-import their respective components: | ||
|
|
||
| ```typescript | ||
| // Before (both from same chunk): | ||
| { id: "notes", component: () => import("@/apps/NotesApp").then(m => ({ default: m.NotesApp })) } | ||
| { id: "todo", component: () => import("@/apps/NotesApp").then(m => ({ default: m.TodoApp })) } | ||
|
|
||
| // After (separate chunks): | ||
| { id: "notes", component: () => import("@/apps/NotesApp").then(m => ({ default: m.NotesApp })) } | ||
| { id: "todo", component: () => import("@/apps/TodoApp").then(m => ({ default: m.TodoApp })) } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve the app-registry contradiction.
Line 260 says no registry changes are needed, but the target block changes the Todo lazy import from NotesApp to TodoApp, and C2 explicitly requires updating the imports. Revise the “No changes” statement so the implementation does not omit this routing change.
🤖 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 `@docs/design/notes-todo-split.md` around lines 260 - 270, Revise the
app-registry guidance to explicitly require updating the `todo` entry’s lazy
import from `@/apps/NotesApp` to `@/apps/TodoApp`, while leaving the existing
`notes` entry unchanged. Remove or correct the statement that no
`app-registry.ts` changes are needed.
| A one-time migration script at `tinyagentos/migrations/migrate_notes_todo_split.py` | ||
| (or as a store `_post_init` migration): | ||
|
|
||
| **Step 1:** Create `todo_lists` and `todo_items` tables. | ||
|
|
||
| **Step 2:** Copy existing `shared_docs` with `kind = 'list'` into `todo_lists`: | ||
| ```sql | ||
| INSERT INTO todo_lists (id, owner_user_id, title, created_at, updated_at, archived_at) | ||
| SELECT id, owner_user_id, title, created_at, updated_at, archived_at | ||
| FROM shared_docs WHERE kind = 'list'; | ||
| ``` | ||
|
|
||
| **Step 3:** Copy entries: | ||
| ```sql | ||
| INSERT INTO todo_items (id, list_id, text, done, position, author, created_at, updated_at) | ||
| SELECT e.id, e.doc_id, e.text, e.done, | ||
| (SELECT COUNT(*) FROM shared_doc_entries e2 | ||
| WHERE e2.doc_id = e.doc_id AND e2.created_at <= e.created_at) - 1, | ||
| e.author, e.created_at, e.created_at | ||
| FROM shared_doc_entries e | ||
| JOIN shared_docs d ON d.id = e.doc_id | ||
| WHERE d.kind = 'list'; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the migration actually idempotent.
The PR objective requires an idempotent migration, but table creation and both INSERT statements are described as one-shot operations. A rerun after a crash or partial completion can hit the primary keys on todo_lists and todo_items; member migration is also unspecified. Define transaction/checkpoint behavior and explicit conflict handling for every step.
🤖 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 `@docs/design/notes-todo-split.md` around lines 279 - 300, Update the migration
described in migrate_notes_todo_split.py to be safely rerunnable: wrap each
migration phase in a transaction or durable checkpoint, make table creation
non-failing when tables already exist, and add explicit conflict handling to
both todo_lists and todo_items inserts. Also define how membership data is
migrated and checkpointed so crashes or partial completion can resume without
duplicate rows or lost members.
| INSERT INTO todo_items (id, list_id, text, done, position, author, created_at, updated_at) | ||
| SELECT e.id, e.doc_id, e.text, e.done, | ||
| (SELECT COUNT(*) FROM shared_doc_entries e2 | ||
| WHERE e2.doc_id = e.doc_id AND e2.created_at <= e.created_at) - 1, | ||
| e.author, e.created_at, e.created_at | ||
| FROM shared_doc_entries e | ||
| JOIN shared_docs d ON d.id = e.doc_id | ||
| WHERE d.kind = 'list'; | ||
| ``` | ||
|
|
||
| `position` is derived from `created_at` order — items retain their existing | ||
| chronological order after migration. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a deterministic tie-breaker when deriving positions.
The COUNT(... created_at <= e.created_at) expression assigns the same position to entries with equal timestamps. Since Todo reads depend on position, migrated items can have nondeterministic order. Add a stable secondary key, such as id, to the ordering and position calculation.
🤖 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 `@docs/design/notes-todo-split.md` around lines 293 - 304, Update the position
calculation in the shared_doc_entries migration INSERT to use a deterministic
ordering by created_at and a stable secondary key such as id. Ensure the
correlated COUNT for each entry counts rows preceding or equal to it under that
same composite order, producing unique positions while preserving chronological
ordering.
| **Step 4:** Copy members for migrated lists into new todo_members table. | ||
|
|
||
| **Step 5:** Do NOT delete the source data — leave `shared_docs` rows in place | ||
| as a safety net. The migration is additive. A future cleanup PR can remove | ||
| kind=list rows after the split is stable. | ||
|
|
||
| **Step 6:** Remove `kind` column from `shared_docs` (or set all remaining to | ||
| `note`). Drop the `done` column from `shared_doc_entries` after confirming | ||
| no Todo consumers remain on the old tables. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Align collaboration-data migration with the storage design.
The target schema says member and revision data are provided by shared_collaboration, while this migration introduces todo_members and omits revision-history migration entirely. Define whether collaboration rows remain in one shared database or are copied into TodoStore; if copied, migrate both members and revisions while preserving IDs and references.
🤖 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 `@docs/design/notes-todo-split.md` around lines 306 - 314, Update the migration
plan around Steps 4–6 to match the storage design: explicitly state whether
collaboration data remains in the shared database or is copied into TodoStore.
If copied, replace the todo_members-only migration with migration of both member
and revision rows, preserving their IDs and all references, and include the
required source/target table handling.
| **Step 5:** Do NOT delete the source data — leave `shared_docs` rows in place | ||
| as a safety net. The migration is additive. A future cleanup PR can remove | ||
| kind=list rows after the split is stable. | ||
|
|
||
| **Step 6:** Remove `kind` column from `shared_docs` (or set all remaining to | ||
| `note`). Drop the `done` column from `shared_doc_entries` after confirming | ||
| no Todo consumers remain on the old tables. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not drop kind while retaining list source rows.
Step 5 preserves kind='list' rows as a safety net, but Step 6 says to remove kind or rewrite remaining rows to note. Rewriting migrated Todo rows as notes misclassifies the retained source data, while dropping the column removes the distinction. Define a cleanup order that deletes or archives legacy list rows before removing kind.
🤖 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 `@docs/design/notes-todo-split.md` around lines 308 - 314, Revise Step 6 in the
migration plan so legacy shared_docs rows with kind='list' are first deleted or
archived after confirming the split is stable. Only then remove the kind column
or normalize remaining rows to note, preserving the distinction for retained
source data until cleanup is complete.
| 3. **Phase 3 (C3):** Agent tools — `todo_list_lists`, `todo_add_item`, | ||
| `todo_set_done`. Remove `notes_set_done`. | ||
| 4. **Phase 4 (C4):** Cleanup — remove `kind` from Notes API, drop kind=list | ||
| support from `/api/notes`, remove old `shared_doc_entries.done` column. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make notes_set_done removal timing consistent.
Phase 3 and C3 remove notes_set_done, while the risk mitigation says the old tool remains registered until C4. Choose one cutover point and document the compatibility behavior; otherwise agents may lose the existing tool before the final cleanup phase.
Also applies to: 366-370, 388-390
🤖 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 `@docs/design/notes-todo-split.md` around lines 322 - 325, Align the
`notes_set_done` removal timing across the Phase 3/C3 plan and the related
risk-mitigation sections. Choose a single cutover point, document whether the
existing tool remains registered as a compatibility path until that point, and
ensure all references consistently describe the same behavior through C4.
Addresses Kilo suggestion: note in timestamp conversion docs that datetime.fromisoformat() handles Z suffix natively on Python >=3.11 (the project's minimum version).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/design/notes-todo-split.md (1)
145-177: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSpecify atomic reorder invariants.
Define whether reorder requests must contain every item exactly once, require unique positions, and execute atomically. Otherwise partial payloads or duplicate positions can leave stale or nondeterministic ordering.
🤖 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 `@docs/design/notes-todo-split.md` around lines 145 - 177, Update the ReorderItemsIn/ReorderEntry reorder specification to require every list item exactly once, enforce unique positions, and apply the entire reorder atomically. Document validation and failure behavior so invalid or partial requests make no changes and valid requests produce deterministic ordering.
🤖 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.
Outside diff comments:
In `@docs/design/notes-todo-split.md`:
- Around line 145-177: Update the ReorderItemsIn/ReorderEntry reorder
specification to require every list item exactly once, enforce unique positions,
and apply the entire reorder atomically. Document validation and failure
behavior so invalid or partial requests make no changes and valid requests
produce deterministic ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: df271e5f-6694-45d2-8abe-76c01b2742d4
📒 Files selected for processing (1)
docs/design/notes-todo-split.md
Document that reorder requests must include every item exactly once with unique positions, execute atomically in a single transaction, and reject invalid payloads with 400.
|
All review feedback addressed across 3 commits: Commit 1 (45e100a): Move file + 4 original Kilo nits
Commit 2 (e5cd60f): Kilo cycle-2 suggestion
Commit 3 (f94ecf0): CodeRabbit cycle-2 suggestion
Bot reviews addressed. Ready for final review. |
| ``` | ||
|
|
||
| **Reorder contract:** The reorder endpoint receives a complete ordering of | ||
| every item in the list — each `id` must appear exactly once, `position` |
There was a problem hiding this comment.
SUGGESTION: Reorder contract leaves two edge cases unspecified.
- The contract rejects payloads containing "missing items," but if a client sends a stale snapshot (e.g. an item was added concurrently by another user), the entire reorder is rejected with 400. Consider documenting optimistic-concurrency behavior or that the payload is validated only against the ids it contains.
- It requires "unique positions (no ties)" but doesn't state whether positions must be a contiguous
0..N-1range or merely distinct integers. A store that re-normalizes to contiguous positions vs one that stores arbitrary unique ints will behave differently on later inserts. Specify the expected range soPUT reorderandPOST items(appended at end) stay consistent.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
Two Kilo suggestions to fold before this lands, both on notes-todo-split.md:
Docs-only otherwise looks good, will merge once these two are addressed. |
2 participants
Design doc for splitting Notes and Todo into fully separate apps per #1923.
Decisions
Next
C1–C5 implement per this design.
Summary by CodeRabbit