Add editor display and title-bar settings - #199
Conversation
📝 WalkthroughWalkthroughThe PR persists editor display preferences and sidebar sorting settings. It adds controls for title-bar metadata, toolbar visibility, editor-width resizing, note sorting, and folder-section collapse. It also adds Vitest configuration and tests. ChangesSettings contracts and utility behavior
Editor preferences
Sidebar navigation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/notes/FolderTreeView.tsx (1)
779-845: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix:
Enteron the "Folders" disclosure button is silently blocked.
SidebarFolderSection(rendered at lines 904-934) introduces the first native, Tab-focusable<button>inside the keyboard-nav container.focusedItemKeynever tracks this button, socurrentIndexis-1when it has focus.handleKeyDowncallse.preventDefault()unconditionally forEnteroncevisibleItems.length > 0, before checkingcurrentIndex. This cancels the button's nativeEnteractivation. A keyboard user who tabs to the "Folders" button cannot toggle it withEnter; only a mouse click orSpaceworks.Move the
currentIndex < 0guard forEnterbefore thepreventDefault()/stopPropagation()calls, so the container only interceptsEnterwhen a tracked item is focused.🐛 Proposed fix
if (visibleItems.length === 0) return; - e.preventDefault(); - e.stopPropagation(); - - const currentIndex = visibleItems.findIndex( + + const currentIndex = visibleItems.findIndex( (item) => itemKey(item) === focusedItemKey, ); + + if (e.key === "Enter" && currentIndex < 0) return; + + e.preventDefault(); + e.stopPropagation();Add a regression test that renders
FolderTreeView, tabs focus to the "Folders" disclosure button, and assertsEntertogglesfoldersSectionCollapsed. Do you want me to draft 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.tsx` around lines 779 - 845, Move the currentIndex < 0 guard into the Enter branch of handleKeyDown before the unconditional preventDefault and stopPropagation calls, while preserving interception for tracked items. Add a regression test for FolderTreeView that tabs to the Folders disclosure button and verifies Enter toggles foldersSectionCollapsed.
🧹 Nitpick comments (2)
src/components/editor/EditorWidthHandle.test.tsx (1)
13-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test case for the actual container-width clamp.
The current mock returns
width: 526withclientWidth: 600, soMath.min(renderedWidth, container.clientWidth)never reaches the clamped branch. The test name implies it verifies the cap against an "unconstrained max-width," but no assertion currently proves that arenderedWidthlarger thanclientWidthgets capped.Add a case where the mocked
getBoundingClientRectwidth exceedsclientWidth, and assert the result equalsclientWidth.✅ Proposed additional test case
it("measures the rendered page instead of its unconstrained max-width", () => { ... expect(getRenderedEditorWidth(container)).toBe(526); }); + + it("caps the measured width at the container width", () => { + const container = document.createElement("div"); + const editor = document.createElement("div"); + editor.className = "ProseMirror"; + editor.getBoundingClientRect = () => ({ + x: 0, + y: 0, + left: 0, + top: 0, + right: 700, + bottom: 800, + width: 700, + height: 800, + toJSON: () => ({}), + }); + Object.defineProperty(container, "clientWidth", { value: 600 }); + container.append(editor); + + expect(getRenderedEditorWidth(container)).toBe(600); + });🤖 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/editor/EditorWidthHandle.test.tsx` around lines 13 - 33, Add a separate test case alongside the existing “measures the rendered page” test that mocks getBoundingClientRect in the container setup with a width greater than clientWidth, then assert getRenderedEditorWidth returns the container’s clientWidth, covering the clamp branch.vitest.config.ts (1)
1-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the
IS_REACT_ACT_ENVIRONMENTsetup in a Vitest setup file. Two test files duplicate the sameglobalThis.IS_REACT_ACT_ENVIRONMENT = trueassignment. The root cause is thatvitest.config.tshas nosetupFilesentry to set this once for all React-rendering tests.
vitest.config.ts#L1-L10: add atest.setupFilesentry pointing to a new setup file that setsglobalThis.IS_REACT_ACT_ENVIRONMENT = trueonce.src/components/editor/EditorWidthHandle.test.tsx#L9-L10: remove the inline assignment now covered by the shared setup file.src/components/settings/EditorSettingsSection.test.tsx#L10-L11: remove the inline assignment now covered by the shared setup file.🤖 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 `@vitest.config.ts` around lines 1 - 10, Centralize the React act environment setup by adding a Vitest setup file referenced through test.setupFiles in vitest.config.ts, with that file assigning globalThis.IS_REACT_ACT_ENVIRONMENT = true. Remove the duplicate inline assignments from src/components/editor/EditorWidthHandle.test.tsx lines 9-10 and src/components/settings/EditorSettingsSection.test.tsx lines 10-11; no direct changes are needed elsewhere.
🤖 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/Sidebar.tsx`:
- Around line 176-210: Update the settings persistence used by
handleNoteSortOrderChange so changing sidebarSortOrder cannot overwrite
concurrent settings updates: prefer making update_settings merge only supplied
fields into the existing settings, or use an atomic/current-setting update that
rejects stale full-object writes. Preserve the optimistic UI update and rollback
behavior while ensuring concurrent settings changes are retained.
In `@src/context/ThemeContext.tsx`:
- Around line 235-238: Update updateSettingsPatch to serialize read-merge-write
operations through a shared promise queue, ensuring each invocation awaits
completion of the previous updateSettings call before calling getSettings.
Preserve the existing patch merge behavior and ensure the queue remains usable
after both successful and failed writes.
---
Outside diff comments:
In `@src/components/notes/FolderTreeView.tsx`:
- Around line 779-845: Move the currentIndex < 0 guard into the Enter branch of
handleKeyDown before the unconditional preventDefault and stopPropagation calls,
while preserving interception for tracked items. Add a regression test for
FolderTreeView that tabs to the Folders disclosure button and verifies Enter
toggles foldersSectionCollapsed.
---
Nitpick comments:
In `@src/components/editor/EditorWidthHandle.test.tsx`:
- Around line 13-33: Add a separate test case alongside the existing “measures
the rendered page” test that mocks getBoundingClientRect in the container setup
with a width greater than clientWidth, then assert getRenderedEditorWidth
returns the container’s clientWidth, covering the clamp branch.
In `@vitest.config.ts`:
- Around line 1-10: Centralize the React act environment setup by adding a
Vitest setup file referenced through test.setupFiles in vitest.config.ts, with
that file assigning globalThis.IS_REACT_ACT_ENVIRONMENT = true. Remove the
duplicate inline assignments from
src/components/editor/EditorWidthHandle.test.tsx lines 9-10 and
src/components/settings/EditorSettingsSection.test.tsx lines 10-11; no direct
changes are needed elsewhere.
🪄 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: 04d3b667-bc8c-404e-87dd-3daf5c266e5b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
package.jsonsrc-tauri/src/lib.rssrc/components/editor/Editor.tsxsrc/components/editor/EditorWidthHandle.test.tsxsrc/components/editor/EditorWidthHandle.tsxsrc/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/components/settings/EditorSettingsSection.test.tsxsrc/components/settings/EditorSettingsSection.tsxsrc/context/ThemeContext.tsxsrc/lib/editorToolbar.test.tssrc/lib/editorToolbar.tssrc/lib/editorWidthResize.test.tssrc/lib/editorWidthResize.tssrc/lib/folderTree.test.tssrc/lib/folderTree.tssrc/lib/titleBarNoteInfo.test.tssrc/lib/titleBarNoteInfo.tssrc/types/note.tsvitest.config.ts
| setNoteSortOrder( | ||
| s.sidebarSortOrder === "oldest" ? "oldest" : "newest", | ||
| ); | ||
| }).catch((error) => { | ||
| console.error("Failed to load settings:", error); | ||
| setFoldersEnabled(false); | ||
| }); | ||
| }, []); | ||
|
|
||
| const handleNoteSortOrderChange = useCallback( | ||
| (nextSortOrder: NoteSortOrder) => { | ||
| if (nextSortOrder === noteSortOrder) return; | ||
|
|
||
| const previousSortOrder = noteSortOrder; | ||
| setNoteSortOrder(nextSortOrder); | ||
|
|
||
| void notesService | ||
| .getSettings() | ||
| .then((settings) => | ||
| notesService.updateSettings({ | ||
| ...settings, | ||
| sidebarSortOrder: nextSortOrder, | ||
| }), | ||
| ) | ||
| .catch((error) => { | ||
| console.error("Failed to save note sort order:", error); | ||
| setNoteSortOrder((current) => | ||
| current === nextSortOrder ? previousSortOrder : current, | ||
| ); | ||
| toast.error("Failed to save note sort order"); | ||
| }); | ||
| }, | ||
| [noteSortOrder], | ||
| ); | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether update_settings performs a full overwrite or a partial merge.
rg -n -B2 -A 30 'fn update_settings' src-tauri/src/lib.rsRepository: erictli/scratch
Length of output: 1275
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'notesService|settings|Settings' . | sed 's#^\./##' | head -100
echo
echo "== references to notesService and settings APIs =="
rg -n "notesService\.(getSettings|updateSettings)|updateSettings\(|getSettings\(|update_settings|sidebarSortOrder" . --glob '!node_modules' --glob '!dist' --glob '!build' | head -200
echo
echo "== lib.rs relevant settings functions =="
sed -n '1680,1790p' src-tauri/src/lib.rsRepository: erictli/scratch
Length of output: 9232
Protect settings updates from lost concurrent writes.
handleNoteSortOrderChange reads the full settings object, then updateSettings sends it to update_settings, where *settings = new_settings. This replaces the in-process settings object. If another settings update runs between the read and write, that update can be lost. Apply a partial merge in update_settings or require the caller/atomic updater to include the current setting and reject stale updates.
🤖 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 176 - 210, Update the
settings persistence used by handleNoteSortOrderChange so changing
sidebarSortOrder cannot overwrite concurrent settings updates: prefer making
update_settings merge only supplied fields into the existing settings, or use an
atomic/current-setting update that rejects stale full-object writes. Preserve
the optimistic UI update and rollback behavior while ensuring concurrent
settings changes are retained.
…ings' into agent/editor-display-titlebar-settings
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/context/ThemeContext.tsx (1)
726-798: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftSplit the provider data and actions into separate contexts.
contextValuecombines all settings data and all setter callbacks. A change to one appearance setting changes the provider value and rerenders action-only consumers. Use the repository dual-context pattern to separate data from actions.As per coding guidelines,
src/**/*{Context,context,Notes}.{ts,tsx}must use theNotesContextdual context pattern with data and actions separated.🤖 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/context/ThemeContext.tsx` around lines 726 - 798, Refactor the ThemeContext provider around contextValue to follow the repository’s dual-context pattern: create separate data and actions contexts, placing theme/settings values in the data context and setter/reset/reload callbacks in the actions context. Update the provider exports and consumers to use the appropriate context, and ensure each context has only the dependencies relevant to its contents.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@src/context/ThemeContext.tsx`:
- Around line 726-798: Refactor the ThemeContext provider around contextValue to
follow the repository’s dual-context pattern: create separate data and actions
contexts, placing theme/settings values in the data context and
setter/reset/reload callbacks in the actions context. Update the provider
exports and consumers to use the appropriate context, and ensure each context
has only the dependencies relevant to its contents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ce43b6f-f9d3-4f69-a984-3f5457f38ee4
📒 Files selected for processing (7)
src/components/editor/EditorWidthHandle.test.tsxsrc/components/layout/SidebarControls.tsxsrc/context/ThemeContext.test.tsxsrc/context/ThemeContext.tsxsrc/lib/settingsUpdateQueue.test.tssrc/lib/settingsUpdateQueue.tssrc/lib/titleBarNoteInfo.test.ts
💤 Files with no reviewable changes (1)
- src/components/layout/SidebarControls.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/titleBarNoteInfo.test.ts
1 participant
Summary
Stack
This is PR 2 of the Scratch 1.0.1 backport stack and depends on #198.
The branch currently includes the PR 1 commit. Merge #198 first; this PR diff will then shrink to the editor display and title-bar settings commit.
Verification
npm test -- --run: 9 files, 29 tests passed.npm run build: passed.cargo test --manifest-path src-tauri/Cargo.toml --quiet: 2 tests passed.cargo clippy --manifest-path src-tauri/Cargo.toml --all-targets --all-features -- -D warnings: passed.git diff --check HEAD^..HEAD: passed.Tests cover donor defaults and migration behavior, disabled resize pointer behavior, formatting-toolbar visibility, accessible settings controls, title-bar mode exclusivity, filename formatting, and empty-state handling.
Scope
Standalone Markdown windows, Settings access from standalone windows, multiple workspaces, selection formatting, tables, and drag/drop remain outside this change.
No zero-bug guarantee is possible. Risk is reduced through focused regression tests, the complete frontend suite available at this stack level, production build, Rust serialization tests, Clippy, and a scope-leak audit.
Summary by CodeRabbit