-
Notifications
You must be signed in to change notification settings - Fork 304
feat: add unified app settings persistence #57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,9 +5,117 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //! MCP server status from the running McpClient. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use std::path::PathBuf; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use std::sync::atomic::{AtomicBool, Ordering}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use serde::{Deserialize, Serialize}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| static SETTINGS_CHANGED: AtomicBool = AtomicBool::new(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub fn settings_changed() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| SETTINGS_CHANGED.store(true, Ordering::SeqCst); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub fn has_settings_changed() -> bool { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| SETTINGS_CHANGED.swap(false, Ordering::SeqCst) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Unified app settings that persist across restarts. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[serde(rename_all = "camelCase")] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub struct AppSettings { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Currently active model key from _models/config.yaml | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub active_model_key: Option<String>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Allowed filesystem paths for sandboxed operations | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub allowed_paths: Vec<String>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// UI theme preference | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub theme: String, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Whether to show tool traces | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub show_tool_traces: bool, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Sampling config (integrated from existing system) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub sampling: SamplingConfig, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| impl Default for AppSettings { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn default() -> Self { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Self { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| active_model_key: None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| allowed_paths: Vec::new(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| theme: "system".to_string(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| show_tool_traces: true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sampling: SamplingConfig::default(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Default allowed paths | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| impl AppSettings { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const FILE_NAME: &'static str = "settings.json"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn persist_path() -> PathBuf { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| crate::data_dir().join(Self::FILE_NAME) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub fn load_or_default() -> Self { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let path = Self::persist_path(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if !path.exists() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return Self::default(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| match std::fs::read_to_string(&path) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Ok(content) => match serde_json::from_str::<Self>(&content) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Ok(settings) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tracing::info!(path = %path.display(), "loaded app settings"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| settings | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Err(e) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tracing::warn!(error = %e, "failed to parse settings, using defaults"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Self::default() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Err(e) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tracing::warn!(error = %e, "failed to read settings, using defaults"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Self::default() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+61
to
+76
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return Self::default(); | |
| } | |
| match std::fs::read_to_string(&path) { | |
| Ok(content) => match serde_json::from_str::<Self>(&content) { | |
| Ok(settings) => { | |
| tracing::info!(path = %path.display(), "loaded app settings"); | |
| settings | |
| } | |
| Err(e) => { | |
| tracing::warn!(error = %e, "failed to parse settings, using defaults"); | |
| Self::default() | |
| } | |
| }, | |
| Err(e) => { | |
| tracing::warn!(error = %e, "failed to read settings, using defaults"); | |
| Self::default() | |
| // No settings.json yet; still hydrate sampling from its own persisted state. | |
| let mut settings = Self::default(); | |
| settings.sampling = SamplingConfig::load_or_default(); | |
| return settings; | |
| } | |
| match std::fs::read_to_string(&path) { | |
| Ok(content) => match serde_json::from_str::<Self>(&content) { | |
| Ok(mut settings) => { | |
| // Always hydrate sampling from the canonical SamplingConfig storage | |
| // to avoid stale or divergent sampling settings. | |
| settings.sampling = SamplingConfig::load_or_default(); | |
| tracing::info!(path = %path.display(), "loaded app settings"); | |
| settings | |
| } | |
| Err(e) => { | |
| tracing::warn!(error = %e, "failed to parse settings, using defaults"); | |
| let mut settings = Self::default(); | |
| settings.sampling = SamplingConfig::load_or_default(); | |
| settings | |
| } | |
| }, | |
| Err(e) => { | |
| tracing::warn!(error = %e, "failed to read settings, using defaults"); | |
| let mut settings = Self::default(); | |
| settings.sampling = SamplingConfig::load_or_default(); | |
| settings |
Copilot
AI
Mar 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
save() returns () and the Tauri commands return AppSettings even if persisting fails (errors are only logged). This makes it hard for the frontend to surface failures (e.g., permissions/IO errors) and can lead to UI showing settings as “saved” when they weren’t. Consider changing save()/update commands to return Result<_, String> and propagating a useful error message to the caller.
Copilot
AI
Mar 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import_from_json writes the imported sampling config to disk, but it doesn’t update the in-memory SamplingConfig stored in Tauri state. As a result, send_message will continue using the old sampling values until restart or until update_sampling_config is called. Consider making import_settings accept the sampling state and update it (and/or consolidate sampling persistence so there’s a single authoritative location).
| settings.sampling.save(); |
Copilot
AI
Mar 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update_app_settings persists only settings.json via settings.save(). If the frontend updates settings.sampling, the new values won’t be applied to the SamplingConfig held in Tauri state (used by send_message), and won’t be written to sampling_config.json unless another command is called. Consider making this command update the TokioMutex<SamplingConfig> state and call settings.sampling.save() as part of the same update (or remove sampling from AppSettings if it’s intentionally separate).
Copilot
AI
Mar 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All AppSettings mutations currently follow a load-modify-save pattern without any shared in-memory lock/state. Since Tauri commands can run concurrently, simultaneous updates (e.g., add_allowed_path racing update_app_settings) can easily overwrite each other’s changes on disk (“last writer wins”). Consider managing a single TokioMutex<AppSettings> in Tauri state (loaded once at startup) so updates are serialized and persisted from the locked value.
Copilot
AI
Mar 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove_allowed_path creates path_clone solely for logging, but path is still available after retain() (it’s only borrowed there). Removing the extra clone avoids an unnecessary allocation on every call.
| let path_clone = path.clone(); | |
| settings.allowed_paths.retain(|p| p != &path); | |
| settings.save(); | |
| tracing::info!(path = %path_clone, "allowed path removed"); | |
| settings.allowed_paths.retain(|p| p != &path); | |
| settings.save(); | |
| tracing::info!(path = %path, "allowed path removed"); |
Copilot
AI
Mar 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
poll_settings_changed() only reflects calls to settings_changed() within this process (currently only AppSettings::save()), so it won’t detect external edits to settings.json and also won’t flip when SamplingConfig is changed via update_sampling_config. If the intent is “external change detection” as described, consider implementing an mtime/hash check against the settings file (or clarify/rename this to reflect it’s an in-process dirty flag).
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -34,6 +34,11 @@ pub(crate) fn data_dir() -> std::path::PathBuf { | |||||||
| .join(".localcowork") | ||||||||
| } | ||||||||
|
|
||||||||
| /// Returns the cache directory for the app (embedding indexes, etc.). | ||||||||
|
||||||||
| /// Returns the cache directory for the app (embedding indexes, etc.). | |
| /// Returns the cache directory for the app (embedding indexes, etc.). | |
| #[allow(dead_code)] // Currently unused; kept for future embedding/cache storage integration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment
// Default allowed pathsinAppSettings::default()is currently misleading because no defaults are actually added (the function returns immediately). Either remove the comment or implement the intended default path seeding so the code and comment stay consistent.