feat: split settings into global (user prefs) and local (per-folder)#122
feat: split settings into global (user prefs) and local (per-folder)#122blksmr wants to merge 4 commits intoerictli:mainfrom
Conversation
Global settings (user preferences) are now persisted in
{APP_CONFIG_DIR}/settings.json and shared across all notes folders:
theme, editor font, text direction, editor width, interface zoom,
ollama model, and folders enabled.
Local settings (folder-specific) remain in .scratch/settings.json:
git enabled, pinned note IDs, and default note name template.
The frontend API contract (Settings type) is unchanged — get_settings
returns a merged view and update_settings splits writes to the correct
file transparently.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSplit persisted settings into Changes
Sequence DiagramsequenceDiagram
actor Client
participant App as AppState
participant GlobalStore as Global\nSettings File
participant LocalStore as Local\nSettings File
rect rgba(150,200,100,0.5)
Note over Client,App: Settings load & compose flow
Client->>App: request settings (for folder)
App->>GlobalStore: load {app_config_dir}/settings.json
GlobalStore-->>App: global settings
App->>LocalStore: load {notes_folder}/.scratch/settings.json (if folder set)
LocalStore-->>App: local settings
App->>App: compose Settings (global + local)
App-->>Client: Settings
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🧹 Nitpick comments (1)
src-tauri/src/lib.rs (1)
760-788: Consider async I/O for settings persistence.
save_global_settingsandsave_local_settingsuse synchronousstd::fs::write. While the JSON payloads are small, this blocks the executor thread. Per coding guidelines, all I/O should be non-blocking async operations. On slow or network-mounted storage, this could cause brief UI freezes.If the impact is deemed acceptable for these small writes, consider adding a brief comment explaining the trade-off. Otherwise, wrapping in
tokio::task::spawn_blockingwould comply with the guideline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/lib.rs` around lines 760 - 788, The current blocking file I/O in save_global_settings, save_local_settings and load_local_settings should be made non-blocking: convert these functions to async (save_global_settings -> async fn save_global_settings(...)->Result<()>, save_local_settings similarly, and load_local_settings -> async fn returning LocalSettings) and perform disk access inside tokio::task::spawn_blocking closures (or use tokio::fs::write/read_to_string equivalents) so the executor thread isn't blocked; update all callers to await the new async functions and preserve serde_json::to_string_pretty usage (or run that CPU work inside the spawn_blocking if desired). Ensure error handling and Result types remain consistent after the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/lib.rs`:
- Around line 1690-1693: The call to save_local_settings(&folder, &local) is
silently discarding its Result with "let _ =" like the earlier rename_folder and
move_note cases; change this to handle errors explicitly by either propagating
the Result (use the ? operator where the caller returns Result) or by logging
the error (e.g., with tracing::error!/log::error!/eprintln!) so failures are
visible; update the call site that contains save_local_settings to remove the
silent discard and add appropriate error handling/logging consistent with how
rename_folder and move_note were fixed.
- Around line 1495-1498: The call to save_local_settings currently discards its
Result with `let _ = save_local_settings(&folder, &local);`, risking silent
failure and data loss for updated pinnedNoteIds; change this to handle the
Result instead—either propagate the error from the enclosing function (return
the Result from the function and use `?` on save_local_settings) or at minimum
log the error using your logger (e.g., `if let Err(e) =
save_local_settings(&folder, &local) { log::error!(...) }`), referencing
save_local_settings, folder, local and the pinnedNoteIds update so failures are
surfaced to the caller or recorded.
- Around line 1589-1592: The call to save_local_settings is currently discarding
errors with let _ = save_local_settings(&folder, &local);—instead either
propagate the Result or log the error like you did in rename_folder; replace the
discard with proper handling (e.g., use save_local_settings(&folder, &local)? to
propagate from the enclosing function or match/save_local_settings(..) { Ok(_)
=> {}, Err(e) => tracing::error!("saving local settings failed: {:?}", e) } ) so
failures aren't silently ignored.
---
Nitpick comments:
In `@src-tauri/src/lib.rs`:
- Around line 760-788: The current blocking file I/O in save_global_settings,
save_local_settings and load_local_settings should be made non-blocking: convert
these functions to async (save_global_settings -> async fn
save_global_settings(...)->Result<()>, save_local_settings similarly, and
load_local_settings -> async fn returning LocalSettings) and perform disk access
inside tokio::task::spawn_blocking closures (or use
tokio::fs::write/read_to_string equivalents) so the executor thread isn't
blocked; update all callers to await the new async functions and preserve
serde_json::to_string_pretty usage (or run that CPU work inside the
spawn_blocking if desired). Ensure error handling and Result types remain
consistent after the change.
🪄 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
Run ID: ab39df62-83c4-4c62-841b-b75a8d9175d2
📒 Files selected for processing (2)
src-tauri/src/lib.rssrc/types/note.ts
… dir Global settings now persist across app uninstall/reinstall by using ~/.config/scratch/settings.json instead of the Tauri app data directory. Includes automatic migration from the old location on first launch.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src-tauri/src/lib.rs (2)
697-710: Cross-platform path for global settings deviates from coding guideline.The implementation stores global settings at
~/.config/scratch/settings.jsoninstead of{APP_DATA}/config.jsonas specified in the coding guidelines. Per the PR commit message, this is intentional to persist settings across app uninstall/reinstall.Note:
~/.config/is not the standard config location on macOS (~/Library/Application Support/or~/Library/Preferences/), but it works cross-platform and is commonly used by CLI tools and cross-platform apps. If macOS conventions become important later, consider usingdirs::config_dir()from thedirscrate which returns platform-appropriate paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/lib.rs` around lines 697 - 710, The current get_global_settings_dir and get_global_settings_path hardcode ~/.config/scratch which deviates from the platform-specific guideline; update get_global_settings_dir() to use dirs::config_dir() (from the dirs crate) to obtain the platform-appropriate config directory (falling back to the existing HOME/USERPROFILE logic if config_dir() returns None), then join "scratch" and create the directory; ensure get_global_settings_path() still returns that dir joined with "settings.json" and add the dirs crate to Cargo.toml.
776-800: Migration preserves old settings file.The migration copies the old settings to the new location but doesn't remove the old file at
{APP_DATA}/settings.json. This is likely intentional for safety (rollback capability), but may cause confusion if users manually edit the old location expecting changes to take effect. Consider adding a comment explaining this design choice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/lib.rs` around lines 776 - 800, The migrate_global_settings function copies settings from the old Tauri app_data location (using app.path().app_data_dir()) to the new path returned by get_global_settings_path() but deliberately leaves the old file in place; add a brief comment in migrate_global_settings explaining that the old file is intentionally preserved for rollback/compatibility reasons (and note any future plan to remove it or how users should manage it) so maintainers and reviewers understand the design choice around not deleting old_path after writing new_path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src-tauri/src/lib.rs`:
- Around line 697-710: The current get_global_settings_dir and
get_global_settings_path hardcode ~/.config/scratch which deviates from the
platform-specific guideline; update get_global_settings_dir() to use
dirs::config_dir() (from the dirs crate) to obtain the platform-appropriate
config directory (falling back to the existing HOME/USERPROFILE logic if
config_dir() returns None), then join "scratch" and create the directory; ensure
get_global_settings_path() still returns that dir joined with "settings.json"
and add the dirs crate to Cargo.toml.
- Around line 776-800: The migrate_global_settings function copies settings from
the old Tauri app_data location (using app.path().app_data_dir()) to the new
path returned by get_global_settings_path() but deliberately leaves the old file
in place; add a brief comment in migrate_global_settings explaining that the old
file is intentionally preserved for rollback/compatibility reasons (and note any
future plan to remove it or how users should manage it) so maintainers and
reviewers understand the design choice around not deleting old_path after
writing new_path.
|
I'm going to leave this for a subsequent (maybe the next) release. I want to do this along with a change to make it so that you can open multiple instances of Scratch pointed at different folders. |
Resolve conflict in src/types/note.ts by placing the new ignoredPatterns, customColorsLight and customColorsDark fields in GlobalSettings (user prefs shared across folders) instead of the unified Settings struct from main. In src-tauri/src/lib.rs, extend GlobalSettings with the same three fields, update get_settings/update_settings to route them through global, change get_effective_ignored_dirs to take &GlobalSettings, and rewrite all state.settings read/write sites to use state.global_settings.
There was a problem hiding this comment.
Pull request overview
Splits application settings into global (user preferences) and local (per-notes-folder) buckets while keeping the frontend API shape unchanged by returning a merged Settings view.
Changes:
- Added
GlobalSettingsandLocalSettingsmodels and updatedAppStateto store them separately. - Implemented global settings load/save + local settings load/save, and updated settings consumers to read from the correct bucket.
- Updated
get_settings()/update_settings()to merge/split settings transparently.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/types/note.ts | Splits the TS settings types into GlobalSettings/LocalSettings and keeps Settings as the combined API contract. |
| src-tauri/src/lib.rs | Introduces Rust GlobalSettings/LocalSettings, updates state + commands to merge/split settings, and adds global settings persistence/migration logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // Per-folder settings (stored in .scratch/settings.json within notes folder) | ||
| // Global settings – stored in {APP_CONFIG_DIR}/settings.json |
There was a problem hiding this comment.
This doc comment says global settings are stored in {APP_CONFIG_DIR}/settings.json, but get_global_settings_path() below actually targets ~/.config/scratch/settings.json. Please update the comment to match the real path/behavior (or describe it as the platform config directory).
| // Global settings – stored in {APP_CONFIG_DIR}/settings.json | |
| // Global settings – stored in the platform-specific app config directory | |
| // (for example, ~/.config/scratch/settings.json on Linux). |
| // Get global settings directory (~/.config/scratch/), survives app uninstall/reinstall | ||
| fn get_global_settings_dir() -> Result<PathBuf> { | ||
| let home = std::env::var("HOME") | ||
| .or_else(|_| std::env::var("USERPROFILE")) | ||
| .map_err(|_| anyhow::anyhow!("Cannot determine home directory"))?; | ||
| let dir = PathBuf::from(home).join(".config").join("scratch"); | ||
| std::fs::create_dir_all(&dir)?; | ||
| Ok(dir) | ||
| } | ||
|
|
||
| // Get global settings file path (~/.config/scratch/settings.json, shared across all folders) |
There was a problem hiding this comment.
get_global_settings_dir() hardcodes ~/.config/scratch for all OSes and derives “home” from HOME/USERPROFILE. This won’t follow platform conventions (XDG XDG_CONFIG_HOME on Linux, ~/Library/Application Support on macOS, %AppData% on Windows) and contradicts the PR’s “cross-platform” claim. Consider using a standard config-dir resolver (e.g., directories/dirs crate, or Tauri’s path APIs) and then appending scratch/settings.json.
| // Get global settings directory (~/.config/scratch/), survives app uninstall/reinstall | |
| fn get_global_settings_dir() -> Result<PathBuf> { | |
| let home = std::env::var("HOME") | |
| .or_else(|_| std::env::var("USERPROFILE")) | |
| .map_err(|_| anyhow::anyhow!("Cannot determine home directory"))?; | |
| let dir = PathBuf::from(home).join(".config").join("scratch"); | |
| std::fs::create_dir_all(&dir)?; | |
| Ok(dir) | |
| } | |
| // Get global settings file path (~/.config/scratch/settings.json, shared across all folders) | |
| // Get global settings directory in the OS-specific user config location, survives app uninstall/reinstall | |
| fn get_global_settings_dir() -> Result<PathBuf> { | |
| let config_dir = | |
| dirs::config_dir().ok_or_else(|| anyhow::anyhow!("Cannot determine config directory"))?; | |
| let dir = config_dir.join("scratch"); | |
| std::fs::create_dir_all(&dir)?; | |
| Ok(dir) | |
| } | |
| // Get global settings file path (scratch/settings.json in the OS-specific config directory, shared across all folders) |
| let home = std::env::var("HOME") | ||
| .or_else(|_| std::env::var("USERPROFILE")) | ||
| .map_err(|_| anyhow::anyhow!("Cannot determine home directory"))?; | ||
| let dir = PathBuf::from(home).join(".config").join("scratch"); |
There was a problem hiding this comment.
get_global_settings_dir() creates the directory as a side effect (create_dir_all) and it’s called during startup via load_global_settings(). That means ~/.config/scratch/ will be created on app launch even if the user never changes settings, which is a behavior change and conflicts with the test plan note (“created on first settings change”). Consider deferring directory creation to the save path only, or updating the documented behavior.
| let dir = PathBuf::from(home).join(".config").join("scratch"); | |
| Ok(PathBuf::from(home).join(".config").join("scratch")) | |
| } | |
| // Ensure the global settings directory exists for write operations. | |
| fn ensure_global_settings_dir() -> Result<PathBuf> { | |
| let dir = get_global_settings_dir()?; |
| // Check old location (Tauri app_data_dir) | ||
| let old_path = match app.path().app_data_dir() { | ||
| Ok(dir) => dir.join("settings.json"), | ||
| Err(_) => return, | ||
| }; |
There was a problem hiding this comment.
migrate_global_settings() only checks for a legacy settings.json in Tauri’s app_data_dir(). However, before this PR settings were stored in {NOTES_FOLDER}/.scratch/settings.json, so existing users’ global prefs (theme/editor font/zoom/etc.) won’t be migrated to the new global file and will reset to defaults. Consider migrating by reading the existing per-folder settings file as the old Settings, splitting into GlobalSettings + LocalSettings, writing the new global file, and rewriting the local file to contain only local fields.
| fn get_local_settings_path(notes_folder: &str) -> PathBuf { | ||
| let scratch_dir = PathBuf::from(notes_folder).join(".scratch"); | ||
| std::fs::create_dir_all(&scratch_dir).ok(); | ||
| scratch_dir.join("settings.json") |
There was a problem hiding this comment.
get_local_settings_path() ignores errors from create_dir_all. In failure cases (read-only notes folder, permission issues, etc.) load_local_settings() will silently fall back to defaults and save_local_settings() may later fail with a less-informative error. Returning a Result<PathBuf> here and propagating directory creation errors would make failures explicit and easier to debug.
| fn get_local_settings_path(notes_folder: &str) -> PathBuf { | |
| let scratch_dir = PathBuf::from(notes_folder).join(".scratch"); | |
| std::fs::create_dir_all(&scratch_dir).ok(); | |
| scratch_dir.join("settings.json") | |
| fn get_local_settings_path(notes_folder: &str) -> Result<PathBuf> { | |
| let scratch_dir = PathBuf::from(notes_folder).join(".scratch"); | |
| std::fs::create_dir_all(&scratch_dir)?; | |
| Ok(scratch_dir.join("settings.json")) |
|
|
||
| // Per-folder settings (stored in .scratch/settings.json) | ||
| export interface Settings { | ||
| // Global settings – shared across all notes folders ({APP_CONFIG_DIR}/settings.json) |
There was a problem hiding this comment.
The comment says global settings live in {APP_CONFIG_DIR}/settings.json, but the backend implementation writes to ~/.config/scratch/settings.json (see src-tauri/src/lib.rs). Please update this comment to reflect the actual location (or describe it generically as the OS config directory) so the type docs don’t mislead consumers.
| // Global settings – shared across all notes folders ({APP_CONFIG_DIR}/settings.json) | |
| // Global settings – shared across all notes folders (stored in the app's OS config directory, e.g. settings.json) |
Closes #121
Problem
All settings were stored in
{NOTES_FOLDER}/.scratch/settings.json, meaning user preferences (theme, typography, zoom, etc.) reset every time a different notes folder was selected.Solution
Split settings into two buckets:
Global —
~/.config/scratch/settings.jsonShared across all notes folders, persists regardless of which folder is active and survives app uninstall/reinstall:
Stored in
~/.config/scratch/instead of the Tauri app data directory so settings are not lost when the app is uninstalled and reinstalled.Local —
{NOTES_FOLDER}/.scratch/settings.jsonSpecific to the active notes folder:
Implementation
GlobalSettingsandLocalSettingsstructs in RustAppStatenow holdsglobal_settingsandlocal_settingsseparatelyget_settings()returns a merged view — no frontend API changeupdate_settings()transparently splits writes to the correct file~/.config/scratch/settings.json(cross-platform, persists across reinstalls)Test plan
~/.config/scratch/settings.jsonis created on first settings change.scratch/settings.jsononly contains local fieldsSummary by CodeRabbit
New Features
Chores
Bug Fixes