refactor(desktop): split NotesApp into notes-only component with shared module#2037
refactor(desktop): split NotesApp into notes-only component with shared module#2037hognek wants to merge 1 commit into
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: 48 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 (6)
✨ 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}
| className="flex items-center gap-1 rounded-md text-shell-text-secondary transition-colors hover:text-shell-text md:hidden"> | ||
| <ChevronLeft size={16} /> | ||
| </button> | ||
| <h2 className="flex-1 truncate text-sm font-semibold text-shell-text">{doc.title}</h2> |
There was a problem hiding this comment.
WARNING: Standalone Todo loses the Share/members panel and per-entry revision history that the previous DocsApp-based list variant provided.
The old TODO_CONFIG ran through the shared DocsApp, which rendered SharePanel (with member add/remove) and EntryHistoryPanel for every entry. This new TodoApp has no Share button in the detail header and TodoItemRow has no history affordance, and TodoDetail/TodoItem types carry no members/revisions fields. For existing kind="list" docs that had collaborators, sharing is now silently unavailable after this refactor.
If /api/todo does not yet support members or entry history, that is fine, but it should be an explicit, documented decision rather than an accidental regression. Consider noting it in the PR description or tracking follow-up work.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| try { | ||
| const r = await fetch(API_BASE); | ||
| if (r.ok) { const data: unknown = await r.json(); setLists(Array.isArray(data) ? (data as TodoList[]) : []); } | ||
| } catch { /* keep whatever was loaded */ } finally { setLoading(false); } |
There was a problem hiding this comment.
SUGGESTION: loadLists swallows fetch failures with an empty catch and only flips loading to false. If GET /api/todo errors (network down, 500), the list pane stays on the "Loading..." spinner forever with no error surfaced to the user.
The shared DocsApp.loadNotes has the same pattern, so this is consistent, but since this is new code it would be good to add an error state (like TodoDetailPane already does) so failures are visible rather than an infinite spinner.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| @@ -23,7 +23,8 @@ vi.mock("@/components/ui", () => ({ | |||
| ), | |||
| })); | |||
There was a problem hiding this comment.
SUGGESTION: Todo tests now exist in two places: this file (__tests__/NotesApp.test.tsx) and the top-level desktop/src/apps/NotesApp.test.tsx, both of which import { TodoApp } from "../TodoApp" (or ./TodoApp) and define a describe("TodoApp") block.
Having two parallel test suites for the same component is confusing and risks drift. Consider consolidating the Todo tests into a single file (e.g. a dedicated TodoApp.test.tsx) and removing the duplicate describe("TodoApp") from one of them.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (6 files)
Prior findings (from previous review, verified at HEAD
|
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| WARNING | 1 |
| SUGGESTION | 2 |
Issue Details (click to expand)
WARNING
| File | Line | Issue |
|---|---|---|
desktop/src/apps/TodoApp.tsx |
257 | Standalone Todo drops the Share/members panel and per-entry revision history that the prior DocsApp list variant had — possible silent regression for shared lists |
SUGGESTION
| File | Line | Issue |
|---|---|---|
desktop/src/apps/TodoApp.tsx |
348 | loadLists has no error state; on fetch failure the list pane shows an infinite "Loading..." spinner |
desktop/src/apps/__tests__/NotesApp.test.tsx |
24 | Todo tests are duplicated across two files (__tests__/NotesApp.test.tsx and NotesApp.test.tsx) — consolidate to avoid drift |
Files Reviewed (6 files)
desktop/src/apps/NotesApp.test.tsxdesktop/src/apps/NotesApp.tsxdesktop/src/apps/TodoApp.tsxdesktop/src/apps/__tests__/NotesApp.test.tsxdesktop/src/apps/notes-shared-base.tsxdesktop/src/registry/app-registry.ts
Reviewed by hy3:free · Input: 94.7K · Output: 12.3K · Cached: 1.2M
|
Fold list before merge:
Once folded and CI green I will merge. Big refactor so I will diff the extracted components for behavior parity. |
…load error state, duplicate test suites - Add comment acknowledging Share/members panel not yet supported by /api/todo (WARNING) - Add loadError state to loadLists with retry UI instead of swallowing failures (SUGGESTION) - Consolidate duplicate TodoApp test suites into __tests__/NotesApp.test.tsx (SUGGESTION) PR jaylfc#2037
…load error state, duplicate test suites - Add comment acknowledging Share/members panel not yet supported by /api/todo (WARNING) - Add loadError state to loadLists with retry UI instead of swallowing failures (SUGGESTION) - Consolidate duplicate TodoApp test suites into __tests__/NotesApp.test.tsx (SUGGESTION) PR jaylfc#2037 Docs-Reviewed: README already lists both Notes and Todo under platform apps; standalone TodoApp.tsx extraction just makes the component boundary match the docs
9e7d225 to
f4ed777
Compare
…load error state, duplicate test suites
- WARNING: Added explicit comment documenting that standalone TodoApp does not yet carry Share/members panel or per-entry revision history from the old DocsApp list variant — tracked as follow-up work
- SUGGESTION: Added loadError state + retry button in loadLists — fetch failures now show an error alert instead of infinite spinner
- SUGGESTION: Consolidated Todo tests into __tests__/NotesApp.test.tsx, removed duplicate describe("TodoApp") from NotesApp.test.tsx
Docs-Reviewed: README already lists both Notes and Todo under platform apps; standalone TodoApp.tsx extraction just makes the component boundary match the docs
f4ed777 to
248c28f
Compare
| } | ||
|
|
||
| if (loading) return <div className="flex h-full items-center justify-center"><p className="text-sm text-shell-text-tertiary">Loading...</p></div>; | ||
| if (error || !doc) return <div className="flex h-full items-center justify-center"><p className="text-sm text-red-400" role="alert">{error ?? "List not found."}</p></div>; |
There was a problem hiding this comment.
WARNING: Item-operation failures blank the entire detail pane
The shared error state is set by addItem, deleteItem, editItem, and toggleDone on failure, but the early return if (error || !doc) then replaces the whole pane (task list + add form) with only the error message. There is no retry or dismiss control, and loadDoc does not clear error, so the pane is stuck on the error until the list is remounted. A transient failure on any single item operation hides all tasks with no recovery path.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const inputRef = useRef<HTMLInputElement>(null); | ||
| useEffect(() => { inputRef.current?.focus(); }, []); | ||
|
|
||
| async function create() { |
There was a problem hiding this comment.
WARNING: create() never resets creating or clears title on success
On the success path setCreating(false) only runs in catch, and title is never reset. After a successful create the Create button stays disabled showing "Creating..." and the input keeps the old title. The shared CreateNoteForm in notes-shared-base.tsx carries the identical defect, so a fix here should be mirrored there.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
2 participants
Summary
Split the monolithic NotesApp.tsx into a shared module + two thin app wrappers:
notes-shared-base.tsx — shared DocsApp component, sub-components (EntryHistoryPanel, SharePanel, EntryRow, NoteListItem, CreateNoteForm, NoteDetailPane, MemberBadge), types, and helpers. Extracted from the original NotesApp.tsx.
NotesApp.tsx — thin notes-only wrapper (33 lines). Hardcodes kind="note" config. No more kind-branching or TodoApp export.
TodoApp.tsx — standalone todo component. Uses the dedicated /api/todo store. Originally created alongside PR feat(todo): add TodoStore + /api/todo routes with ordering and due dates #1944 but never committed.
app-registry.ts — todo app now points to
@/apps/TodoAppinstead of importing TodoApp from@/apps/NotesApp.Data migration
Existing docs with kind="note" continue to work without changes. Docs with kind="list" are handled by TodoApp via /api/todo. No database migration required.
Tests
Task: t_f5920047