Add sidebar sorting and folder collapse controls - #198
Conversation
📝 WalkthroughWalkthroughThe sidebar now supports persisted newest-first or oldest-first note sorting and a persisted collapsible folders section. Vitest, ChangesSidebar navigation behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant NoteSortMenu
participant Sidebar
participant updateSidebarSortOrder
participant NoteList
User->>NoteSortMenu: Select newest or oldest
NoteSortMenu->>Sidebar: onChange(sortOrder)
Sidebar->>updateSidebarSortOrder: Save sort order
updateSidebarSortOrder-->>Sidebar: Save result
Sidebar->>NoteList: Pass sortOrder
NoteList-->>User: Render sorted notes
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/layout/Sidebar.tsx (1)
172-209: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAvoid read-modify-write on the full
Settingsobject.
updateSettings(newSettings)replaces the in-memorySettingsstate and writes that object to.scratch/settings.json.handleNoteSortOrderChangereads settings, then writes onlysidebarSortOrder, so any overlapping frontend update that rewrites the same full object can drop the sidebar sort change; examples include pin/unpin settings updates and othergetSettings()-then-updateSettings()paths. Add a Rust-side field patch command such asupdate_sidebar_sort_order, or serialize frontend settings writes so they cannot interleave.🤖 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 `@src/components/layout/Sidebar.tsx` around lines 172 - 209, Update handleNoteSortOrderChange and the settings update flow to avoid full Settings read-modify-write for sidebarSortOrder. Prefer adding and invoking a Rust-side field patch command such as update_sidebar_sort_order so concurrent settings updates cannot overwrite this change; otherwise serialize all frontend getSettings-then-updateSettings writes, including pin/unpin paths.
🧹 Nitpick comments (1)
src/components/notes/FolderTreeView.test.tsx (1)
1-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for section-reopening and keyboard-visibility behavior.
This file only tests that
FolderItemComponentexposes no descendant-collapse action. It does not testFolderTreeView'sexpandFolderbehavior, which resetsfoldersSectionCollapsedtofalseso that drag-and-drop, subfolder creation, and search navigation all reopen a collapsed Folders section. It also does not test thatgetVisibleItemsForFolderSectionexcludes folder items from keyboard navigation while the section is collapsed.The PR objective calls out "focused tests for ... keyboard navigation" for this layer. Add a test that collapses the section, triggers an
expand-folderevent (or calls the exposed reopening path), and asserts the section becomes expanded and keyboard navigation reaches folder items again.Do you want me to generate this test?
🤖 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 `@src/components/notes/FolderTreeView.test.tsx` around lines 1 - 67, Extend FolderTreeView tests beyond FolderItemComponent to cover section reopening: collapse the Folders section, trigger the expandFolder/“expand-folder” path, and assert foldersSectionCollapsed becomes false. Also verify getVisibleItemsForFolderSection excludes folder items while collapsed and includes them after reopening, confirming keyboard navigation can reach folders again.
🤖 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 `@src/components/layout/SidebarFolderSection.tsx`:
- Around line 4-35: The PR objective must accurately describe
foldersSectionCollapsed persistence: update it to say device/local storage
because loadFolderSectionCollapsed and saveFolderSectionCollapsed use
globalThis.localStorage, or alternatively add foldersSectionCollapsed to the
Settings model and route persistence through notesService.updateSettings if it
must be exported or synced with Scratch settings.
---
Outside diff comments:
In `@src/components/layout/Sidebar.tsx`:
- Around line 172-209: Update handleNoteSortOrderChange and the settings update
flow to avoid full Settings read-modify-write for sidebarSortOrder. Prefer
adding and invoking a Rust-side field patch command such as
update_sidebar_sort_order so concurrent settings updates cannot overwrite this
change; otherwise serialize all frontend getSettings-then-updateSettings writes,
including pin/unpin paths.
---
Nitpick comments:
In `@src/components/notes/FolderTreeView.test.tsx`:
- Around line 1-67: Extend FolderTreeView tests beyond FolderItemComponent to
cover section reopening: collapse the Folders section, trigger the
expandFolder/“expand-folder” path, and assert foldersSectionCollapsed becomes
false. Also verify getVisibleItemsForFolderSection excludes folder items while
collapsed and includes them after reopening, confirming keyboard navigation can
reach folders again.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 85e3075e-e578-487d-a12c-99e972348292
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
package.jsonsrc-tauri/src/lib.rssrc/components/layout/Sidebar.tsxsrc/components/layout/SidebarControls.test.tsxsrc/components/layout/SidebarControls.tsxsrc/components/layout/SidebarFolderSection.test.tsxsrc/components/layout/SidebarFolderSection.tsxsrc/components/notes/FolderTreeView.test.tsxsrc/components/notes/FolderTreeView.tsxsrc/components/notes/NoteList.tsxsrc/lib/folderTree.test.tssrc/lib/folderTree.tssrc/types/note.tsvitest.config.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/components/layout/SidebarControls.test.tsx`:
- Around line 63-80: Replace the innerHTML assertion in the “does not prevent
Radix focus restoration on the sort trigger” test with a behavioral
focus-restoration check: open and close the sort menu, then assert that focus
returns to the sort trigger. Use the existing NoteSortMenu interaction path and
preserve cleanup of the rendered root.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 672b603c-c20b-4e9f-aacc-532d9dcb49ed
📒 Files selected for processing (14)
src-tauri/src/lib.rssrc/components/layout/Sidebar.tsxsrc/components/layout/SidebarControls.test.tsxsrc/components/layout/SidebarControls.tsxsrc/components/layout/SidebarFolderSection.test.tsxsrc/components/layout/SidebarFolderSection.tsxsrc/components/notes/FolderTreeView.test.tsxsrc/components/notes/FolderTreeView.tsxsrc/components/notes/NoteList.tsxsrc/lib/folderTree.test.tssrc/lib/folderTree.tssrc/services/notes.tssrc/test/setup.tsvitest.config.ts
💤 Files with no reviewable changes (1)
- src/components/layout/SidebarControls.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/layout/SidebarFolderSection.test.tsx
- src/components/layout/Sidebar.tsx
- vitest.config.ts
- src/components/notes/FolderTreeView.tsx
| it("does not prevent Radix focus restoration on the sort trigger", () => { | ||
| const onChange = vi.fn(); | ||
| const container = document.createElement("div"); | ||
| document.body.append(container); | ||
| const root = createRoot(container); | ||
|
|
||
| act(() => { | ||
| root.render( | ||
| <TooltipProvider> | ||
| <NoteSortMenu sortOrder="newest" onChange={onChange} /> | ||
| </TooltipProvider>, | ||
| ); | ||
| }); | ||
|
|
||
| const source = container.innerHTML; | ||
| expect(source).not.toContain("onCloseAutoFocus"); | ||
|
|
||
| act(() => root.unmount()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test focus restoration through behavior.
Line 77 cannot detect onCloseAutoFocus. React does not serialize event props into innerHTML. This test will pass if a future change adds that handler. Close the menu and assert that focus returns to the sort trigger.
🤖 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 `@src/components/layout/SidebarControls.test.tsx` around lines 63 - 80, Replace
the innerHTML assertion in the “does not prevent Radix focus restoration on the
sort trigger” test with a behavioral focus-restoration check: open and close the
sort menu, then assert that focus returns to the sort trigger. Use the existing
NoteSortMenu interaction path and preserve cleanup of the rendered root.
1 participant
Summary
.scratch/settings.json.localStorage.Verification
npm test -- --run: 4 files, 8 tests passed.npm run build: passed.cargo test --manifest-path src-tauri/Cargo.toml --quiet: 1 test passed.cargo clippy --manifest-path src-tauri/Cargo.toml --all-targets --all-features -- -D warnings: passed.git diff --check origin/main...HEAD: passed.Tests cover newest/oldest ordering, deterministic ties, identical ordering in flat and folder views, persisted folder-section collapse, accessible controls, and keyboard-navigation exclusion while the section is collapsed.
Scope
This is PR 1 and the base of the Scratch 1.0.1 backport stack. Editor preferences, standalone windows, workspaces, formatting menus, tables, and drag/drop remain outside this change.
No zero-bug guarantee is possible, but the branch is independent, buildable, and covered by focused regression tests.
Summary by CodeRabbit
New Features
Bug Fixes
Tests