From bd21f78534f53455042e882a519e0631f60fb0a7 Mon Sep 17 00:00:00 2001 From: Jakob Stender Guldberg Date: Wed, 2 Sep 2026 12:11:44 +0200 Subject: [PATCH] feat(config): XDG base directories and persistent UI preferences Per-user state was hand-rolled as $HOME/.diffcore/... in five independent places -- the global config, the refinement cache, the comment store, the desktop log and the embeddings cache -- with no XDG support and no Windows fallback, so on Windows (which release.yml ships as MSI and NSIS bundles) HOME is normally unset and none of it resolved at all. Add a paths module resolving config/data/cache/state roots, and route all five call sites through it. XDG variables are honoured as-is on every unix including macOS: ~/.config/diffcore rather than ~/Library/Application Support, matching git and most CLI tooling, which is why this hand-rolls the resolution instead of pulling in the dirs crate. Relative $XDG_*_HOME values are ignored per spec. The pure resolve_from() core takes the environment as arguments so the unit tests never touch process env. Migration is read-fallback only, for the two things a user would notice losing: the global config and review comments. Writes always go to the new paths and nothing is moved or deleted, so a downgrade still works. Comments are user-authored review notes rather than regenerable data, so they land in $XDG_DATA_HOME, not the cache root the spec declares safe to delete; load_cached_comments_file returns an empty list on any read error, so an unreachable file would have looked like deletion rather than an error. Persist theme and panel layout, which previously lived only in localStorage and React state. localStorage is scoped to a browser origin, so running diffcore-web on a different --port silently lost the theme, and panel width and collapse state were lost on every reload. These now live in ui.toml, served by get_ui_settings/save_ui_settings over both the Tauri IPC and the web dispatch. localStorage is kept purely as a synchronous first-paint cache so the theme does not flash on load. ui.toml is deliberately a separate file from config.toml. UI preferences are written on every theme toggle and panel resize; config.toml holds a plaintext API key and is written rarely. Sharing one file would make each of those writes a read-modify-write of the credential, and with the desktop app and diffcore-web both running -- the configuration this feature exists to support -- a theme change could drop a key the other instance had just written. Keeping them apart removes that interleaving rather than mitigating it. config.toml is now written atomically (temp file + rename) and created 0600; fs::write previously left the key world-readable. theme_mode is an enum so serde rejects unknown values at parse time instead of leaving the frontend to sanitise a free-form string, and right_panel_width is clamped on both read and write -- a hand-edited absurd value rendered an unusable layout, and a negative one failed u32 deserialization and took the whole config down with it. A corrupt ui.toml degrades to defaults rather than erroring; preferences must not be able to break startup. LlmSettings gains api_key_in_config. The settings panel gated the "Clear API key" button on api_key_source equalling the literal "~/.diffcore/config.toml", so any change to where the config lives silently removed the only way to delete a stored plaintext key from the UI. A displayed path is not a sentinel; the flag says what the UI actually needs to know. Covered by three Playwright cases, one of which asserts the control survives an unrecognised path. Relatedly, display_global_config_path() now reports the file actually being read, which during the migration window is the pre-XDG one. It previously named the write target, so the panel pointed at a file that did not exist yet. Fixes a test that set DIFFCORE_GLOBAL_CONFIG_DIR, a variable nothing reads, and therefore overwrote the developer's real global config with defaults on every cargo test. pr_url's repo clone cache, which landed upstream while this was in progress, moves to $XDG_CACHE_HOME/diffcore/repos with the rest; it was the sixth hand-rolled $HOME/.diffcore path. Add pkgs.clippy and pkgs.rustfmt to the devShell; neither was available, so cargo clippy could not run at all. --- AGENTS.md | 4 +- README.md | 14 +- crates/diffcore-core/src/cache.rs | 12 +- crates/diffcore-core/src/config.rs | 346 ++++++++++++++++-- crates/diffcore-core/src/embeddings.rs | 16 +- crates/diffcore-core/src/lib.rs | 1 + crates/diffcore-core/src/llm/mod.rs | 2 +- crates/diffcore-core/src/paths.rs | 147 ++++++++ crates/diffcore-core/src/pr_url.rs | 2 +- crates/diffcore-tauri/src/commands.rs | 113 ++++-- crates/diffcore-tauri/src/main.rs | 9 +- crates/diffcore-tauri/src/web_server.rs | 2 + .../tests/comment_integration.rs | 77 +++- crates/diffcore-tauri/ui/src/App.tsx | 97 ++++- crates/diffcore-tauri/ui/src/mock.ts | 3 +- crates/diffcore-tauri/ui/src/themes.ts | 22 +- crates/diffcore-tauri/ui/src/types.ts | 10 + .../ui/tests/e2e/activity-stream.spec.ts | 9 +- .../ui/tests/e2e/hardening.spec.ts | 3 +- .../ui/tests/e2e/onboarding.spec.ts | 65 +++- docs/file-locations.md | 32 ++ flake.nix | 2 + 22 files changed, 872 insertions(+), 116 deletions(-) create mode 100644 crates/diffcore-core/src/paths.rs create mode 100644 docs/file-locations.md diff --git a/AGENTS.md b/AGENTS.md index 2c815c47..7d0c7d15 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -68,8 +68,8 @@ Large-diff work is a separate evaluation track, not part of the default live-rep - **VCR caching** — record/replay LLM calls for deterministic CI - **LLM-as-judge** — evaluator that scores analysis quality across 5 criteria - **Eval suite** — 5 synthetic fixture codebases, deterministic scoring, 0.89 avg score -- **Config** — `.diffcore.toml` with entrypoint globs, layer names, ignore patterns, LLM settings, refinement settings -- **Logging** — `tracing` + `tracing-subscriber` behind `diffcore-core`'s `logging` feature; `RUST_LOG` sets the filter (default `info`), `DIFFCORE_LOG_FORMAT=json` switches to line-delimited JSON, `DIFFCORE_LOG_FILE` redirects to a file (the desktop app falls back to `~/.diffcore/desktop.log` when stderr is not a terminal, since GUI bundles discard it). Backend activity logs on the `activity` target, IR cache on `ir_cache`. `log`-crate call sites bridge in automatically. +- **Config** — `.diffcore.toml` with entrypoint globs, layer names, ignore patterns, LLM settings, refinement settings; per-user config/cache/state resolved via XDG base dirs in `diffcore-core`'s `paths` module (`$XDG_CONFIG_HOME/diffcore/config.toml`, with `~/.diffcore/config.toml` as a read-only legacy fallback). Theme and panel preferences live in a separate `ui.toml` (`UiConfig`) so high-frequency UI writes never race the credential-bearing `config.toml`; review comments are user data and live under `$XDG_DATA_HOME/diffcore/comments/` with a read-fallback to the pre-XDG location +- **Logging** — `tracing` + `tracing-subscriber` behind `diffcore-core`'s `logging` feature; `RUST_LOG` sets the filter (default `info`), `DIFFCORE_LOG_FORMAT=json` switches to line-delimited JSON, `DIFFCORE_LOG_FILE` redirects to a file (the desktop app falls back to `$XDG_STATE_HOME/diffcore/desktop.log` when stderr is not a terminal, since GUI bundles discard it). Backend activity logs on the `activity` target, IR cache on `ir_cache`. `log`-crate call sites bridge in automatically. ## Tests diff --git a/README.md b/README.md index d578fbdf..f07d829d 100644 --- a/README.md +++ b/README.md @@ -148,9 +148,14 @@ The app auto-discovers git branches, worktrees, and push status on launch. Enter Diffcore now splits configuration into: -- `~/.diffcore/config.toml` for shared LLM/onboarding settings across all repos +- `$XDG_CONFIG_HOME/diffcore/config.toml` (defaults to `~/.config/diffcore/config.toml`) + for shared LLM/onboarding settings across all repos - `.diffcore.toml` in the repo root for project-specific analysis settings +Every other path — UI preferences, comments, caches, logs — is listed in +[docs/file-locations.md](docs/file-locations.md), along with the migration +notes for pre-XDG installs. + New users can usually skip API keys entirely: the desktop app auto-detects `codex` and `claude`, and will use those subscriptions when available. Example global config: @@ -197,8 +202,8 @@ uncertainty = 0.20 When using direct API providers (`anthropic`, `openai`, `gemini`), Diffcore checks for API keys in this order: -1. `key_cmd` in `~/.diffcore/config.toml` or `.diffcore.toml` -2. `key` in `~/.diffcore/config.toml` or `.diffcore.toml` +1. `key_cmd` in `~/.config/diffcore/config.toml` or `.diffcore.toml` +2. `key` in `~/.config/diffcore/config.toml` or `.diffcore.toml` 3. `DIFFCORE_API_KEY` 4. Provider specific env var: `ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, or `GEMINI_API_KEY` @@ -220,7 +225,8 @@ DIFFCORE_LOG_FORMAT=json diffcore analyze --base main 2> analyze.log.jsonl Launched from a terminal the desktop app logs there like the others; launched from Finder or a `.desktop` entry — where stderr is discarded — it falls back to -`~/.diffcore/desktop.log`. `DIFFCORE_LOG_FILE` overrides the path for any binary. +`~/.local/state/diffcore/desktop.log`. `DIFFCORE_LOG_FILE` overrides the path +for any binary. ## Architecture diff --git a/crates/diffcore-core/src/cache.rs b/crates/diffcore-core/src/cache.rs index 0df9f5b0..f886955a 100644 --- a/crates/diffcore-core/src/cache.rs +++ b/crates/diffcore-core/src/cache.rs @@ -114,20 +114,14 @@ pub fn clear_cache(workdir: &Path) -> std::io::Result<()> { Ok(()) } -// ── Global refinement cache (~/.diffcore/cache/refinements/) ── +// ── Global refinement cache ($XDG_CACHE_HOME/diffcore/refinements/) ── /// Resolve the global refinement cache directory. +/// Respects `DIFFCORE_REFINEMENT_CACHE_DIR` for testing. fn refinement_cache_dir() -> Option { std::env::var_os("DIFFCORE_REFINEMENT_CACHE_DIR") .map(PathBuf::from) - .or_else(|| { - std::env::var_os("HOME").map(|home| { - PathBuf::from(home) - .join(".diffcore") - .join("cache") - .join("refinements") - }) - }) + .or_else(|| crate::paths::cache_dir().map(|dir| dir.join("refinements"))) } /// Load a cached refinement result for the given analysis cache key. diff --git a/crates/diffcore-core/src/config.rs b/crates/diffcore-core/src/config.rs index 26ec91be..b529bead 100644 --- a/crates/diffcore-core/src/config.rs +++ b/crates/diffcore-core/src/config.rs @@ -6,7 +6,6 @@ //! See spec §6.2 for the full config file format. use std::collections::HashMap; -use std::env; use std::path::{Path, PathBuf}; use serde::{Deserialize, Serialize}; @@ -141,6 +140,149 @@ impl Default for DiffConfig { } } +/// UI preferences, shared across the desktop app and the web build. +/// +/// Deliberately its own file (`ui.toml`), not a section of `config.toml`. +/// UI prefs are written on every theme toggle and panel resize; `config.toml` +/// holds a plaintext API key and is written rarely. Sharing one +/// read-modify-write file would mean a theme toggle could drop a credential +/// written concurrently by another instance. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct UiConfig { + /// Theme mode. + #[serde(default)] + pub theme_mode: ThemeMode, + /// Theme id used when resolving to light. + #[serde(default = "default_theme_light")] + pub theme_light: String, + /// Theme id used when resolving to dark. + #[serde(default = "default_theme_dark")] + pub theme_dark: String, + /// Width of the right-hand panel in pixels. + #[serde(default = "default_right_panel_width")] + pub right_panel_width: u32, + /// Whether the right-hand panel is collapsed. + #[serde(default)] + pub right_panel_collapsed: bool, +} + +/// How the active theme is chosen. An enum so serde rejects garbage at parse +/// time instead of leaving the frontend to sanitise a free-form string. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] +#[serde(rename_all = "lowercase")] +pub enum ThemeMode { + Light, + #[default] + Dark, + System, +} + +fn default_theme_light() -> String { + "catppuccin-latte".to_string() +} + +fn default_theme_dark() -> String { + "catppuccin-mocha".to_string() +} + +fn default_right_panel_width() -> u32 { + 320 +} + +/// Panel width bounds, mirroring the drag clamp in the UI. A width outside +/// this range renders an unusable layout, so it is clamped on both read and +/// write rather than trusted. +const MIN_PANEL_WIDTH: u32 = 200; +const MAX_PANEL_WIDTH: u32 = 800; + +impl Default for UiConfig { + fn default() -> Self { + Self { + theme_mode: ThemeMode::default(), + theme_light: default_theme_light(), + theme_dark: default_theme_dark(), + right_panel_width: default_right_panel_width(), + right_panel_collapsed: false, + } + } +} + +impl UiConfig { + /// Path to `ui.toml` in the XDG config dir. + pub fn path() -> Option { + crate::paths::config_dir().map(|dir| dir.join("ui.toml")) + } + + /// Load UI preferences, falling back to defaults. + /// + /// Never returns an error: a corrupt or hand-mangled prefs file must not + /// be able to take down the app, so it degrades to defaults. + pub fn load() -> Self { + let Some(path) = Self::path() else { + return Self::default(); + }; + let Ok(contents) = std::fs::read_to_string(&path) else { + return Self::default(); + }; + match toml::from_str::(&contents) { + Ok(mut config) => { + config.clamp(); + config + } + Err(e) => { + tracing::warn!("ignoring unparseable {}: {e}", path.display()); + Self::default() + } + } + } + + /// Write UI preferences to `ui.toml`. + pub fn save(&self) -> Result<(), ConfigError> { + let Some(path) = Self::path() else { + return Err(ConfigError::Validation( + "Could not resolve a config directory for UI settings".to_string(), + )); + }; + let mut config = self.clone(); + config.clamp(); + let toml_str = toml::to_string_pretty(&config).map_err(|e| { + ConfigError::Validation(format!("Failed to serialize UI settings: {}", e)) + })?; + write_atomically(&path, &toml_str) + } + + fn clamp(&mut self) { + self.right_panel_width = self + .right_panel_width + .clamp(MIN_PANEL_WIDTH, MAX_PANEL_WIDTH); + } +} + +/// Write to a sibling temp file then rename, so a reader never observes a +/// half-written file and a crash mid-write cannot truncate the old one. +fn write_atomically(path: &Path, contents: &str) -> Result<(), ConfigError> { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent)?; + } + let tmp = path.with_extension("toml.tmp"); + std::fs::write(&tmp, contents)?; + restrict_permissions(&tmp); + std::fs::rename(&tmp, path)?; + Ok(()) +} + +/// Owner-only permissions. `config.toml` carries a plaintext API key, and +/// `fs::write` would otherwise create it 0644. +fn restrict_permissions(path: &Path) { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let _ = std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)); + } + #[cfg(not(unix))] + let _ = path; +} + /// LLM provider configuration. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] pub struct LlmConfig { @@ -276,27 +418,47 @@ pub enum ConfigError { } impl DiffcoreConfig { - /// Return the global diffcore config path in the user's home directory. + /// Return the global diffcore config path, following XDG base directories. /// - /// Resolution order: - /// 1. `DIFFCORE_CONFIG_HOME` - /// 2. `HOME/.diffcore` + /// Resolution order: `DIFFCORE_CONFIG_HOME`, `XDG_CONFIG_HOME/diffcore`, + /// `HOME/.config/diffcore`. This is always where writes go; see + /// [`Self::load_global`] for the pre-XDG read fallback. pub fn global_config_path() -> Option { - diffcore_config_home().map(|dir| dir.join("config.toml")) + crate::paths::config_dir().map(|dir| dir.join("config.toml")) + } + + /// Pre-XDG global config (`~/.diffcore/config.toml`), read-only. + fn legacy_global_config_path() -> Option { + crate::paths::legacy_dir().map(|dir| dir.join("config.toml")) + } + + /// The global config file actually in use, or `None` when none exists yet. + /// + /// Resolution order matches [`Self::load_global`]: the XDG path, then the + /// pre-XDG `~/.diffcore/config.toml`. Callers displaying "your key lives + /// here" must use this, not [`Self::global_config_path`], which always + /// names the write target and so lies during the migration window. + pub fn global_config_source() -> Option { + Self::global_config_path() + .filter(|path| path.exists()) + .or_else(|| Self::legacy_global_config_path().filter(|path| path.exists())) + } + + /// Whether settings are still being read from the pre-XDG location. + pub fn using_legacy_global_config() -> bool { + Self::global_config_path().is_none_or(|path| !path.exists()) + && Self::legacy_global_config_path().is_some_and(|path| path.exists()) } /// Load configuration from the user's global config file. /// - /// Returns defaults when the file is missing or the home directory cannot be resolved. + /// Reads the XDG path when it exists, otherwise falls back to the pre-XDG + /// `~/.diffcore/config.toml` so existing installs keep their settings. + /// Returns defaults when neither is present. pub fn load_global() -> Result { - if let Some(path) = Self::global_config_path() { - if path.exists() { - Self::from_file(&path) - } else { - Ok(Self::default()) - } - } else { - Ok(Self::default()) + match Self::global_config_source() { + Some(path) => Self::from_file(&path), + None => Ok(Self::default()), } } @@ -327,7 +489,7 @@ impl DiffcoreConfig { } } - /// Load repo-local config and merge in global LLM defaults from `~/.diffcore/config.toml`. + /// Load repo-local config and merge in global LLM defaults from the global config. /// /// Repo-local project settings remain authoritative; only the `[llm]` section falls back /// to the global config when values are not set locally. @@ -338,7 +500,7 @@ impl DiffcoreConfig { Ok(local) } - /// Save configuration to the global diffcore config in the user's home directory. + /// Save configuration to the global diffcore config (always the XDG path). pub fn save_global(&self) -> Result<(), ConfigError> { let Some(config_path) = Self::global_config_path() else { return Err(ConfigError::Validation( @@ -346,14 +508,10 @@ impl DiffcoreConfig { )); }; - if let Some(parent) = config_path.parent() { - std::fs::create_dir_all(parent)?; - } - let toml_str = toml::to_string_pretty(self) .map_err(|e| ConfigError::Validation(format!("Failed to serialize config: {}", e)))?; - std::fs::write(&config_path, toml_str)?; - Ok(()) + // Atomic + owner-only: this file holds a plaintext API key. + write_atomically(&config_path, &toml_str) } /// Validate the configuration for consistency. @@ -572,12 +730,6 @@ impl DiffcoreConfig { } } -pub(crate) fn diffcore_config_home() -> Option { - env::var_os("DIFFCORE_CONFIG_HOME") - .map(PathBuf::from) - .or_else(|| env::var_os("HOME").map(|home| PathBuf::from(home).join(".diffcore"))) -} - #[cfg(test)] #[allow( clippy::unwrap_used, @@ -1534,22 +1686,142 @@ provider = "openai" assert!(local.llm.refinement.enabled); } + /// The whole global-config surface in one test. + /// + /// `DIFFCORE_CONFIG_HOME` and `HOME` are process-global, so every case that + /// needs them lives here rather than in parallel tests that would race. #[test] - fn test_save_global_roundtrip_annotations_enabled() { - // Test that annotations_enabled survives save_global / load_global cycle - let dir = tempfile::tempdir().unwrap(); - std::env::set_var("DIFFCORE_GLOBAL_CONFIG_DIR", dir.path().to_str().unwrap()); + fn test_global_config_roundtrip_legacy_fallback_and_ui() { + let xdg = tempfile::tempdir().unwrap(); + let fake_home = tempfile::tempdir().unwrap(); + let prev_home = std::env::var_os("HOME"); + std::env::set_var("DIFFCORE_CONFIG_HOME", xdg.path()); + std::env::set_var("HOME", fake_home.path()); + + // 1. Nothing anywhere: defaults. + assert_eq!(DiffcoreConfig::global_config_source(), None); + assert_eq!(UiConfig::load(), UiConfig::default()); + + // 2. Only the pre-XDG `~/.diffcore/config.toml` exists: it is read. + let legacy_dir = fake_home.path().join(".diffcore"); + std::fs::create_dir_all(&legacy_dir).unwrap(); + std::fs::write( + legacy_dir.join("config.toml"), + "[llm]\nkey = \"sk-legacy\"\n", + ) + .unwrap(); + let loaded = DiffcoreConfig::load_global().unwrap(); + assert_eq!( + loaded.llm.key.as_deref(), + Some("sk-legacy"), + "legacy config should be read when no XDG config exists" + ); + assert!(DiffcoreConfig::using_legacy_global_config()); + assert_eq!( + DiffcoreConfig::global_config_source(), + Some(legacy_dir.join("config.toml")), + "the migration window must report the file actually in use" + ); - let mut config = DiffcoreConfig::default(); + // 3. Saving writes to the XDG path and leaves the legacy file alone. + let mut config = loaded; config.llm.annotations_enabled = false; config.llm.refinement.enabled = false; config.save_global().unwrap(); + assert!(xdg.path().join("config.toml").exists()); + assert!( + legacy_dir.join("config.toml").exists(), + "legacy config must never be moved or deleted" + ); + + // 4. Everything survives the round trip, and XDG now wins over legacy. let loaded = DiffcoreConfig::load_global().unwrap(); - assert!(!loaded.llm.annotations_enabled, "annotations_enabled should survive save/load"); - assert!(!loaded.llm.refinement.enabled, "refinement.enabled should survive save/load"); + assert!(!loaded.llm.annotations_enabled); + assert!(!loaded.llm.refinement.enabled); + assert_eq!(loaded.llm.key.as_deref(), Some("sk-legacy")); + + // 5. UI prefs round-trip through their own file, untouched by the above. + let ui = UiConfig { + theme_mode: ThemeMode::Light, + right_panel_width: 612, + right_panel_collapsed: true, + ..Default::default() + }; + ui.save().unwrap(); + assert_eq!(UiConfig::load(), ui); + assert!(xdg.path().join("ui.toml").exists()); - std::env::remove_var("DIFFCORE_GLOBAL_CONFIG_DIR"); + // 6. A corrupt prefs file degrades to defaults instead of erroring. + std::fs::write(xdg.path().join("ui.toml"), "not = [valid").unwrap(); + assert_eq!(UiConfig::load(), UiConfig::default()); + + // 7. The config file holding the API key is owner-only. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = std::fs::metadata(xdg.path().join("config.toml")) + .unwrap() + .permissions() + .mode(); + assert_eq!(mode & 0o777, 0o600, "config.toml must not be world-readable"); + } + + // 8. During the migration window the reported source is the real one. + assert_eq!( + DiffcoreConfig::global_config_source(), + Some(xdg.path().join("config.toml")) + ); + assert!(!DiffcoreConfig::using_legacy_global_config()); + + std::env::remove_var("DIFFCORE_CONFIG_HOME"); + match prev_home { + Some(home) => std::env::set_var("HOME", home), + None => std::env::remove_var("HOME"), + } + } + + #[test] + fn ui_config_partial_fields_fall_back_to_defaults() { + let ui: UiConfig = toml::from_str("theme_mode = \"system\"\n").unwrap(); + assert_eq!(ui.theme_mode, ThemeMode::System); + assert_eq!(ui.right_panel_width, 320); + assert_eq!(ui.theme_dark, "catppuccin-mocha"); + } + + #[test] + fn ui_config_rejects_unknown_theme_mode() { + assert!(toml::from_str::("theme_mode = \"neon\"\n").is_err()); + } + + #[test] + fn ui_config_clamps_absurd_panel_width() { + let mut wide: UiConfig = toml::from_str("right_panel_width = 4000000\n").unwrap(); + wide.clamp(); + assert_eq!(wide.right_panel_width, MAX_PANEL_WIDTH); + let mut narrow: UiConfig = toml::from_str("right_panel_width = 1\n").unwrap(); + narrow.clamp(); + assert_eq!(narrow.right_panel_width, MIN_PANEL_WIDTH); + } + + /// A negative width must not be able to break the whole prefs file. + #[test] + fn ui_config_survives_invalid_toml() { + assert!(toml::from_str::("right_panel_width = -5\n").is_err()); + } + + /// `.diffcore.toml` comes from a potentially untrusted cloned repo; it must + /// not be able to carry UI preferences at all. + #[test] + fn project_config_does_not_carry_ui_prefs() { + // Unknown sections are ignored by serde, which is what we want: a + // cloned repo's `[ui]` is inert rather than an error the user must fix. + let config = DiffcoreConfig::from_str("[ui]\ntheme_mode = \"light\"\n").unwrap(); + let reserialized = toml::to_string_pretty(&config).unwrap(); + assert!( + !reserialized.contains("[ui]"), + "UI prefs must not round-trip through project config" + ); } // ── auto_subtrees Tests ── diff --git a/crates/diffcore-core/src/embeddings.rs b/crates/diffcore-core/src/embeddings.rs index f8c764de..1ecd0bec 100644 --- a/crates/diffcore-core/src/embeddings.rs +++ b/crates/diffcore-core/src/embeddings.rs @@ -73,10 +73,9 @@ impl EmbeddingCache { } } - /// Default cache location: `~/.cache/diffcore/embeddings/` + /// Default cache location: `$XDG_CACHE_HOME/diffcore/embeddings/` pub fn default_cache() -> Self { - let dir = dirs_fallback().join("diffcore").join("embeddings"); - Self::new(&dir) + Self::new(&embeddings_cache_dir()) } /// Compute a cache key for a file path + content pair. @@ -136,12 +135,11 @@ impl EmbeddingCache { } } -/// Fallback for cache directory — uses `~/.cache` on all platforms. -fn dirs_fallback() -> PathBuf { - if let Some(home) = std::env::var_os("HOME") { - PathBuf::from(home).join(".cache") - } else { - PathBuf::from("/tmp") +/// Embeddings cache directory, honouring XDG like every other per-user path. +fn embeddings_cache_dir() -> PathBuf { + match crate::paths::cache_dir() { + Some(dir) => dir.join("embeddings"), + None => std::env::temp_dir().join("diffcore-embeddings"), } } diff --git a/crates/diffcore-core/src/lib.rs b/crates/diffcore-core/src/lib.rs index 3142bed1..4c196a21 100644 --- a/crates/diffcore-core/src/lib.rs +++ b/crates/diffcore-core/src/lib.rs @@ -23,6 +23,7 @@ pub mod llm; #[cfg(feature = "logging")] pub mod logging; pub mod output; +pub mod paths; pub mod pipeline; pub mod pr_url; pub mod query_engine; diff --git a/crates/diffcore-core/src/llm/mod.rs b/crates/diffcore-core/src/llm/mod.rs index bda75073..c452b58e 100644 --- a/crates/diffcore-core/src/llm/mod.rs +++ b/crates/diffcore-core/src/llm/mod.rs @@ -35,7 +35,7 @@ use schema::{ /// Errors that can occur during LLM operations. #[derive(Debug, thiserror::Error)] pub enum LlmError { - #[error("No API key found. Set DIFFCORE_API_KEY, configure key_cmd in ~/.diffcore/config.toml or .diffcore.toml, or set provider-specific env var ({0})")] + #[error("No API key found. Set DIFFCORE_API_KEY, configure key_cmd in the global config or .diffcore.toml, or set provider-specific env var ({0})")] NoApiKey(String), #[error("HTTP request failed: {0}")] diff --git a/crates/diffcore-core/src/paths.rs b/crates/diffcore-core/src/paths.rs new file mode 100644 index 00000000..96f9da8e --- /dev/null +++ b/crates/diffcore-core/src/paths.rs @@ -0,0 +1,147 @@ +//! XDG base directory resolution for diffcore's per-user state. +//! +//! Config, cache and state each get their own root, following the XDG Base +//! Directory spec on every unix (macOS included — this matches git/nvim +//! convention rather than `~/Library/Application Support`). `USERPROFILE` is +//! the last resort so Windows resolves to something instead of nothing. +//! +//! Repo-local `/.diffcore/` is project state and is not resolved here. + +use std::path::PathBuf; + +/// Per-user config root: `$DIFFCORE_CONFIG_HOME`, `$XDG_CONFIG_HOME/diffcore`, +/// or `$HOME/.config/diffcore`. +pub fn config_dir() -> Option { + resolve("DIFFCORE_CONFIG_HOME", "XDG_CONFIG_HOME", &[".config"]) +} + +/// Per-user cache root: `$DIFFCORE_CACHE_HOME`, `$XDG_CACHE_HOME/diffcore`, +/// or `$HOME/.cache/diffcore`. +pub fn cache_dir() -> Option { + resolve("DIFFCORE_CACHE_HOME", "XDG_CACHE_HOME", &[".cache"]) +} + +/// Per-user data root: `$DIFFCORE_DATA_HOME`, `$XDG_DATA_HOME/diffcore`, +/// or `$HOME/.local/share/diffcore`. +/// +/// For state the user authored and would be upset to lose. Unlike +/// [`cache_dir`], which the XDG spec declares safe for anything to delete. +pub fn data_dir() -> Option { + resolve("DIFFCORE_DATA_HOME", "XDG_DATA_HOME", &[".local", "share"]) +} + +/// Per-user state root: `$DIFFCORE_STATE_HOME`, `$XDG_STATE_HOME/diffcore`, +/// or `$HOME/.local/state/diffcore`. +pub fn state_dir() -> Option { + resolve( + "DIFFCORE_STATE_HOME", + "XDG_STATE_HOME", + &[".local", "state"], + ) +} + +/// Pre-XDG location (`$HOME/.diffcore`), kept as a read-only fallback so +/// existing installs keep their config. Never written to. +pub fn legacy_dir() -> Option { + home().map(|home| home.join(".diffcore")) +} + +fn home() -> Option { + std::env::var_os("HOME") + .or_else(|| std::env::var_os("USERPROFILE")) + .map(PathBuf::from) + .filter(|path| !path.as_os_str().is_empty()) +} + +fn resolve(override_var: &str, xdg_var: &str, home_suffix: &[&str]) -> Option { + resolve_from( + std::env::var_os(override_var).map(PathBuf::from), + std::env::var_os(xdg_var).map(PathBuf::from), + home(), + home_suffix, + ) +} + +/// Pure core of [`resolve`], with the environment passed in. +/// +/// The XDG spec says a relative `$XDG_*_HOME` is invalid and must be ignored. +fn resolve_from( + override_home: Option, + xdg_home: Option, + home: Option, + home_suffix: &[&str], +) -> Option { + if let Some(path) = override_home.filter(|path| !path.as_os_str().is_empty()) { + return Some(path); + } + if let Some(path) = xdg_home.filter(|path| path.is_absolute()) { + return Some(path.join("diffcore")); + } + let mut path = home?; + for segment in home_suffix { + path.push(segment); + } + path.push("diffcore"); + Some(path) +} + +#[cfg(test)] +#[allow(clippy::unwrap_used)] +mod tests { + use super::*; + use std::path::Path; + + fn config( + override_home: Option<&str>, + xdg: Option<&str>, + home: Option<&str>, + ) -> Option { + resolve_from( + override_home.map(PathBuf::from), + xdg.map(PathBuf::from), + home.map(PathBuf::from), + &[".config"], + ) + } + + #[test] + fn override_wins_over_everything() { + let got = config(Some("/opt/dc"), Some("/xdg"), Some("/home/u")).unwrap(); + assert_eq!(got, Path::new("/opt/dc")); + } + + #[test] + fn xdg_home_gets_diffcore_suffix() { + let got = config(None, Some("/xdg"), Some("/home/u")).unwrap(); + assert_eq!(got, Path::new("/xdg/diffcore")); + } + + #[test] + fn relative_xdg_home_is_ignored() { + let got = config(None, Some("relative/xdg"), Some("/home/u")).unwrap(); + assert_eq!(got, Path::new("/home/u/.config/diffcore")); + } + + #[test] + fn empty_override_falls_through() { + let got = config(Some(""), None, Some("/home/u")).unwrap(); + assert_eq!(got, Path::new("/home/u/.config/diffcore")); + } + + #[test] + fn no_home_resolves_to_nothing() { + assert!(config(None, None, None).is_none()); + } + + #[test] + fn state_suffix_is_nested() { + let got = resolve_from( + None, + None, + Some(PathBuf::from("/home/u")), + &[".local", "state"], + ) + .unwrap(); + assert_eq!(got, Path::new("/home/u/.local/state/diffcore")); + } +} diff --git a/crates/diffcore-core/src/pr_url.rs b/crates/diffcore-core/src/pr_url.rs index a3c5e7f0..b8e02d9a 100644 --- a/crates/diffcore-core/src/pr_url.rs +++ b/crates/diffcore-core/src/pr_url.rs @@ -528,7 +528,7 @@ fn lock_cache(dir: &Path) -> Result { pub fn resolve(pr: &PrUrl) -> Result { let root = std::env::var_os("DIFFCORE_REPO_CACHE_DIR") .map(PathBuf::from) - .or_else(|| crate::config::diffcore_config_home().map(|h| h.join("cache").join("repos"))) + .or_else(|| crate::paths::cache_dir().map(|dir| dir.join("repos"))) .ok_or(PrUrlError::NoCacheDir)?; resolve_in(pr, &root) } diff --git a/crates/diffcore-tauri/src/commands.rs b/crates/diffcore-tauri/src/commands.rs index 89fa27ef..72f84cce 100644 --- a/crates/diffcore-tauri/src/commands.rs +++ b/crates/diffcore-tauri/src/commands.rs @@ -3,7 +3,7 @@ //! Each `#[cfg_attr(feature = "desktop", tauri::command)]` function is callable from the frontend via `invoke()`. use std::collections::HashMap; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{Arc, Mutex}; @@ -19,7 +19,7 @@ use crate::state_shim::State; use crate::activity_stream::{self, ActivityEntry, JobHandle}; use diffcore_core::cache; use diffcore_core::cluster; -use diffcore_core::config::DiffcoreConfig; +use diffcore_core::config::{DiffcoreConfig, UiConfig}; use diffcore_core::entrypoint; use diffcore_core::flow::{self, FlowConfig}; use diffcore_core::git; @@ -1674,7 +1674,7 @@ pub fn get_cached_refinement( Ok(None) } -/// Store a refinement result in the global cache (~/.diffcore/cache/refinements/). +/// Store a refinement result in the global cache ($XDG_CACHE_HOME/diffcore/refinements/). /// /// Stores under both diff-hash key and branch-based key for cross-worktree access. #[cfg_attr(feature = "desktop", tauri::command)] @@ -1793,7 +1793,7 @@ pub fn check_api_key(repo_path: Option) -> Result { /// Get LLM settings from the shared global config plus repo-local overrides. /// -/// Reads `~/.diffcore/config.toml`, merges in any repo-local `[llm]` overrides, resolves +/// Reads the global config, merges in any repo-local `[llm]` overrides, resolves /// CLI/API availability, and returns a unified `LlmSettings` struct for the settings panel. #[cfg_attr(feature = "desktop", tauri::command)] pub fn get_llm_settings(repo_path: Option) -> Result { @@ -1813,6 +1813,8 @@ pub fn get_llm_settings(repo_path: Option) -> Result llm::resolve_api_key(&config.llm, &provider).is_ok(), }; + let api_key_in_config = config.llm.key.as_ref().is_some_and(|k| !k.is_empty()); + let api_key_source = match provider.as_str() { "codex" => match (codex_status.installed, codex_status.authenticated) { (true, true) => "Codex CLI login".to_string(), @@ -1825,9 +1827,7 @@ pub fn get_llm_settings(repo_path: Option) -> Result "Claude Code not installed".to_string(), }, _ if config.llm.key_cmd.is_some() => "key_cmd".to_string(), - _ if config.llm.key.as_ref().is_some_and(|k| !k.is_empty()) => { - "~/.diffcore/config.toml".to_string() - } + _ if api_key_in_config => display_global_config_path(), _ if std::env::var("DIFFCORE_API_KEY").is_ok() => "DIFFCORE_API_KEY".to_string(), _ => { let env_var = match provider.as_str() { @@ -1839,7 +1839,7 @@ pub fn get_llm_settings(repo_path: Option) -> Result) -> Result) -> Result Result<(), CommandError> { let mut config = @@ -1916,7 +1917,7 @@ pub fn save_llm_settings(_repo_path: String, settings: LlmSettings) -> Result<() Ok(()) } -/// Save an API key to `~/.diffcore/config.toml` under `[llm] key = "..."`. +/// Save an API key to the global config under `[llm] key = "..."`. /// /// The key is stored directly in the config file. Precedence is maintained: /// `key_cmd` > `key` (config) > env vars. @@ -1934,7 +1935,7 @@ pub fn save_api_key(_repo_path: String, api_key: String) -> Result<(), CommandEr Ok(()) } -/// Remove the stored API key from `~/.diffcore/config.toml`. +/// Remove the stored API key from the global config. #[cfg_attr(feature = "desktop", tauri::command)] pub fn clear_api_key(_repo_path: String) -> Result<(), CommandError> { let mut config = @@ -1949,6 +1950,26 @@ pub fn clear_api_key(_repo_path: String) -> Result<(), CommandError> { Ok(()) } +/// Get UI preferences from `ui.toml`. +/// +/// Per-user, not per-repo: the same theme and layout apply to every repository +/// and to both the desktop app and the web build. +#[cfg_attr(feature = "desktop", tauri::command)] +pub fn get_ui_settings() -> Result { + Ok(UiConfig::load()) +} + +/// Save UI preferences to `ui.toml`. +/// +/// Deliberately does not touch `config.toml`: that file holds the API key and +/// must not be rewritten on every theme toggle. +#[cfg_attr(feature = "desktop", tauri::command)] +pub fn save_ui_settings(settings: UiConfig) -> Result<(), CommandError> { + settings + .save() + .map_err(|e| CommandError::Config(format!("Failed to save UI settings: {}", e))) +} + /// Get the current ignore paths from `.diffcore.toml`. #[cfg_attr(feature = "desktop", tauri::command)] pub fn get_ignore_paths(repo_path: Option) -> Result, CommandError> { @@ -2301,10 +2322,32 @@ fn preferred_model_for_runtime( } } +/// Path to show the user for "where your settings live". +/// +/// Reports the file actually being read, which during the migration window is +/// the pre-XDG one — [`DiffcoreConfig::global_config_path`] names the write +/// target and would point at a file that does not exist yet. fn display_global_config_path() -> String { - DiffcoreConfig::global_config_path() - .map(|path| path.to_string_lossy().to_string()) - .unwrap_or_else(|| "~/.diffcore/config.toml".to_string()) + DiffcoreConfig::global_config_source() + .or_else(DiffcoreConfig::global_config_path) + .map(|path| abbreviate_home(&path)) + .unwrap_or_else(|| "~/.config/diffcore/config.toml".to_string()) +} + +/// Render `/home/you/.config/...` as `~/.config/...` for display. +fn abbreviate_home(path: &Path) -> String { + let display = path.to_string_lossy().to_string(); + let Some(home) = std::env::var_os("HOME") else { + return display; + }; + let home = home.to_string_lossy().to_string(); + if home.is_empty() { + return display; + } + match display.strip_prefix(&home) { + Some(rest) => format!("~{}", rest), + None => display, + } } /// LLM settings for the UI — surface for the settings panel. @@ -2334,6 +2377,9 @@ pub struct LlmSettings { pub refinement_model: String, /// Where shared LLM settings are stored. pub global_config_path: String, + /// Whether a key is stored in the config file (so the UI can offer to + /// clear it). A flag, not a string comparison against a display path. + pub api_key_in_config: bool, /// Whether Codex CLI is installed. pub codex_available: bool, /// Whether Codex CLI is logged in and ready. @@ -2696,7 +2742,7 @@ fn load_comments_from_file(path: &PathBuf, analysis_hash: &str) -> CommentsFile } // ══════════════════════════════════════════════════════════════════════ -// Branch-based comment cache (~/.diffcore/cache/comments/) +// Branch-based comment store ($XDG_DATA_HOME/diffcore/comments/) // ══════════════════════════════════════════════════════════════════════ /// Resolve the global comment cache directory. @@ -2704,14 +2750,28 @@ fn load_comments_from_file(path: &PathBuf, analysis_hash: &str) -> CommentsFile fn comment_cache_dir() -> Option { std::env::var_os("DIFFCORE_COMMENT_CACHE_DIR") .map(PathBuf::from) - .or_else(|| { - std::env::var_os("HOME").map(|home| { - PathBuf::from(home) - .join(".diffcore") - .join("cache") - .join("comments") - }) - }) + .or_else(|| diffcore_core::paths::data_dir().map(|dir| dir.join("comments"))) +} + +/// Pre-XDG comment location, read-only. Comments are user-authored review +/// notes; silently orphaning them on upgrade would look like data loss. +fn legacy_comment_cache_dir() -> Option { + diffcore_core::paths::legacy_dir().map(|dir| dir.join("cache").join("comments")) +} + +/// Resolve the file holding comments for `cache_key`, preferring the current +/// location and falling back to the pre-XDG one. Writes always use the former. +fn comment_cache_file_for_read(cache_key: &str) -> Option { + let name = format!("{}.json", cache_key); + let current = comment_cache_dir().map(|dir| dir.join(&name)); + if current.as_ref().is_some_and(|path| path.exists()) { + return current; + } + let legacy = legacy_comment_cache_dir().map(|dir| dir.join(&name)); + if legacy.as_ref().is_some_and(|path| path.exists()) { + return legacy; + } + current } /// Compute a cache key for a repo+branch combo. @@ -2776,10 +2836,9 @@ struct CachedCommentsFile { /// Load comments for the current repo+branch from the global cache. fn load_cached_comments_file(cache_key: &str) -> CachedCommentsFile { - let Some(dir) = comment_cache_dir() else { + let Some(path) = comment_cache_file_for_read(cache_key) else { return CachedCommentsFile { comments: vec![] }; }; - let path = dir.join(format!("{}.json", cache_key)); match std::fs::read_to_string(&path) { Ok(data) => serde_json::from_str(&data).unwrap_or(CachedCommentsFile { comments: vec![] }), Err(_) => CachedCommentsFile { comments: vec![] }, @@ -3242,6 +3301,7 @@ mod tests { refinement_provider: "claude".to_string(), refinement_model: "default".to_string(), global_config_path: "~/.diffcore/config.toml".to_string(), + api_key_in_config: false, codex_available: true, codex_authenticated: true, claude_available: true, @@ -4128,6 +4188,7 @@ mod tests { refinement_provider: "codex".to_string(), refinement_model: "default".to_string(), global_config_path: "~/.diffcore/config.toml".to_string(), + api_key_in_config: false, codex_available: false, codex_authenticated: false, claude_available: false, @@ -4153,6 +4214,7 @@ mod tests { refinement_provider: "anthropic".to_string(), refinement_model: "claude-sonnet-4-6".to_string(), global_config_path: "/tmp/config.toml".to_string(), + api_key_in_config: false, codex_available: true, codex_authenticated: true, claude_available: true, @@ -4178,6 +4240,7 @@ mod tests { refinement_provider: "gemini".to_string(), refinement_model: "gemini-2.5-flash".to_string(), global_config_path: "~/.diffcore/config.toml".to_string(), + api_key_in_config: false, codex_available: true, codex_authenticated: false, claude_available: true, diff --git a/crates/diffcore-tauri/src/main.rs b/crates/diffcore-tauri/src/main.rs index 54a4041a..f8c7eed5 100644 --- a/crates/diffcore-tauri/src/main.rs +++ b/crates/diffcore-tauri/src/main.rs @@ -42,9 +42,10 @@ fn main() { // Used only when stderr is not a terminal: GUI bundles discard it (no // console on Windows release, Finder/.desktop launches drop it). Launched // from a terminal, the desktop app still logs there. - // `~/.diffcore` matches the cache convention and is not world-writable. - let log_file = std::env::var_os("HOME") - .map(|home| std::path::PathBuf::from(home).join(".diffcore").join("desktop.log")) + // `$XDG_STATE_HOME/diffcore` is the XDG home for logs and is not + // world-writable; the temp dir is a last resort when it cannot be resolved. + let log_file = diffcore_core::paths::state_dir() + .map(|dir| dir.join("desktop.log")) .unwrap_or_else(|| std::env::temp_dir().join("diffcore-desktop.log")); diffcore_core::logging::init(Some(log_file)); if let Err(e) = tauri::Builder::default() @@ -87,6 +88,8 @@ fn main() { commands::delete_comment, commands::load_comments, commands::export_comments, + commands::get_ui_settings, + commands::save_ui_settings, commands::get_ignore_paths, commands::save_ignore_paths, commands::get_cached_refinement, diff --git a/crates/diffcore-tauri/src/web_server.rs b/crates/diffcore-tauri/src/web_server.rs index 76de9171..120ccc65 100644 --- a/crates/diffcore-tauri/src/web_server.rs +++ b/crates/diffcore-tauri/src/web_server.rs @@ -363,6 +363,8 @@ fn dispatch_sync(cmd: &str, args: &mut Args, app: &AppState) -> Result ok(commands::clear_api_key(opt(args, "repoPath")?)?), + "get_ui_settings" => ok(commands::get_ui_settings()?), + "save_ui_settings" => ok(commands::save_ui_settings(req(args, "settings")?)?), "get_ignore_paths" => ok(commands::get_ignore_paths(opt(args, "repoPath")?)?), "save_ignore_paths" => ok(commands::save_ignore_paths( req(args, "repoPath")?, diff --git a/crates/diffcore-tauri/tests/comment_integration.rs b/crates/diffcore-tauri/tests/comment_integration.rs index 721bb1cc..063538aa 100644 --- a/crates/diffcore-tauri/tests/comment_integration.rs +++ b/crates/diffcore-tauri/tests/comment_integration.rs @@ -3,7 +3,7 @@ //! //! Tests the full lifecycle: save → load → delete → export //! using real git repositories and the `.diffcore/comments.json` file. -//! Also tests branch-based comment caching in `~/.diffcore/cache/comments/`. +//! Also tests the branch-based comment store in `$XDG_DATA_HOME/diffcore/comments/`. //! //! Run with: //! cargo test --test comment_integration @@ -830,3 +830,78 @@ fn refinement_cache_branch_key_deterministic() { assert_eq!(key1, key2); assert_eq!(key1.len(), 64); // SHA-256 hex } + +// ── Pre-XDG comment migration ─────────────────────────────────────── +// +// Comments are user-authored review notes, not regenerable cache. Moving the +// store must not orphan them: `load_cached_comments_file` returns an empty +// list on any read failure, so an unreachable file looks like deletion. + +/// Set HOME + XDG_DATA_HOME (no `DIFFCORE_COMMENT_CACHE_DIR`, which would +/// short-circuit the fallback being tested). Shares ENV_LOCK with the others. +fn with_home_and_data_dir(home: &std::path::Path, data: &std::path::Path, f: F) { + let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let prev_home = std::env::var_os("HOME"); + let prev_data = std::env::var_os("XDG_DATA_HOME"); + std::env::remove_var("DIFFCORE_COMMENT_CACHE_DIR"); + std::env::set_var("HOME", home.as_os_str()); + std::env::set_var("XDG_DATA_HOME", data.as_os_str()); + f(); + match prev_home { + Some(v) => std::env::set_var("HOME", v), + None => std::env::remove_var("HOME"), + } + match prev_data { + Some(v) => std::env::set_var("XDG_DATA_HOME", v), + None => std::env::remove_var("XDG_DATA_HOME"), + } +} + +#[test] +fn legacy_comments_survive_the_move_to_xdg_data_home() { + let home = tempfile::tempdir().unwrap(); + let data = tempfile::tempdir().unwrap(); + let (_repo_tmp, repo_path) = create_test_repo_on_branch("feature-legacy"); + + with_home_and_data_dir(home.path(), data.path(), || { + let key = comment_cache_key(&repo_path).unwrap(); + + // A comment written by a pre-XDG install. + let legacy_dir = home.path().join(".diffcore").join("cache").join("comments"); + std::fs::create_dir_all(&legacy_dir).unwrap(); + // Serialised from the real struct: a hand-written shape that fails to + // deserialise would silently read as "no comments" and pass a weaker test. + let legacy_payload = serde_json::json!({ + "comments": [make_comment("old", "file", "pre-XDG note")], + }); + std::fs::write( + legacy_dir.join(format!("{}.json", key)), + serde_json::to_string(&legacy_payload).unwrap(), + ) + .unwrap(); + + let loaded = load_comments_cached(repo_path.clone()).unwrap(); + assert_eq!(loaded.len(), 1, "pre-XDG comments must remain reachable"); + assert_eq!(loaded[0].text, "pre-XDG note"); + + // Writing goes to the new location; the old file is left alone. + let comment = make_comment("new", "code", "post-XDG note"); + save_comment_cached(repo_path.clone(), comment).unwrap(); + assert!( + data.path() + .join("diffcore") + .join("comments") + .join(format!("{}.json", key)) + .exists(), + "writes must go to XDG_DATA_HOME" + ); + assert!( + legacy_dir.join(format!("{}.json", key)).exists(), + "the pre-XDG file must never be moved or deleted" + ); + + // Once the new file exists it wins. + let loaded = load_comments_cached(repo_path.clone()).unwrap(); + assert!(loaded.iter().any(|c| c.id == "new")); + }); +} diff --git a/crates/diffcore-tauri/ui/src/App.tsx b/crates/diffcore-tauri/ui/src/App.tsx index 1c197bd2..7b2c1735 100644 --- a/crates/diffcore-tauri/ui/src/App.tsx +++ b/crates/diffcore-tauri/ui/src/App.tsx @@ -18,6 +18,7 @@ import type { ReviewComment, CommentInput, InfraSubGroup, + UiConfig, } from "./types"; import { LLM_PROVIDERS, MODELS_BY_PROVIDER } from "./types"; import DiffViewer, { type DiffViewerHandle } from "./components/DiffViewer"; @@ -27,11 +28,14 @@ import SourceExplorer, { type SourceFocusRequest } from "./components/SourceExpl // import RiskHeatmap from "./components/RiskHeatmap"; import ErrorBoundary from "./components/ErrorBoundary"; import { buildManifestPrompt } from "./buildManifestPrompt"; -import { THEMES, applyTheme, getTheme, loadThemePrefs, saveThemePrefs, resolveThemeId, type ThemeMode, type ThemePrefs } from "./themes"; +import { THEMES, applyTheme, getTheme, loadThemePrefs, saveThemePrefs, normalizeThemePrefs, resolveThemeId, type ThemeMode, type ThemePrefs } from "./themes"; import { MOCK_ANALYSIS, MOCK_DIFFS, MOCK_PASS2, MOCK_REPO_INFO, MOCK_LLM_SETTINGS, MOCK_REFINEMENT, MOCK_RESOLVED_PR } from "./mock"; import { IS_TAURI, HAS_BACKEND, DEFAULT_REPO, invoke as tauriInvoke } from "./backend"; +/** Panel width bounds, mirroring MIN/MAX_PANEL_WIDTH in diffcore-core. */ +const clampPanelWidth = (width: number) => Math.max(200, Math.min(800, width)); + const PROVIDER_LABELS: Record = { codex: "Codex CLI", claude: "Claude Code", @@ -286,13 +290,70 @@ export default function App() { applyTheme(activeThemeId); }, [activeThemeId]); + // Global config is the source of truth for UI prefs; localStorage is only a + // synchronous cache so the first paint uses the right theme. Demo mode has no + // backend, so it stays localStorage-only. + const uiSettingsRef = useRef(null); + // Set as soon as the user changes a pref. Guards both directions of the + // load race: before the fetch lands we must not write (we'd overwrite the + // fields we haven't read yet), and once the user has acted the resolved + // fetch must not revert them. + const uiTouchedRef = useRef(false); + + const persistUiSettings = useCallback((patch: Partial) => { + if (!HAS_BACKEND) return; + uiTouchedRef.current = true; + const current = uiSettingsRef.current; + // No baseline yet: writing would clobber unread fields with guessed + // defaults. localStorage still holds the theme, and the next change + // after load persists everything. + if (!current) return; + const next = { ...current, ...patch }; + uiSettingsRef.current = next; + // Fire-and-forget: a failed write must never block the interaction. + tauriInvoke("save_ui_settings", { settings: next }).catch((e) => { + console.warn("Failed to persist UI settings", e); + }); + }, []); + + useEffect(() => { + if (!HAS_BACKEND) return; + let cancelled = false; + tauriInvoke("get_ui_settings") + .then((ui) => { + if (cancelled) return; + uiSettingsRef.current = ui; + if (uiTouchedRef.current) return; + const prefs = normalizeThemePrefs({ + mode: ui.theme_mode, + light: ui.theme_light, + dark: ui.theme_dark, + }); + setThemePrefs(prefs); + saveThemePrefs(prefs); + setRightPanelWidth(clampPanelWidth(ui.right_panel_width)); + setRightPanelCollapsed(ui.right_panel_collapsed); + }) + .catch((e) => { + console.warn("Failed to load UI settings; using local defaults", e); + }); + return () => { + cancelled = true; + }; + }, []); + const updateThemePrefs = useCallback((patch: Partial) => { setThemePrefs((prev) => { const next = { ...prev, ...patch }; saveThemePrefs(next); + persistUiSettings({ + theme_mode: next.mode, + theme_light: next.light, + theme_dark: next.dark, + }); return next; }); - }, []); + }, [persistUiSettings]); // LLM settings const [llmSettings, setLlmSettings] = useState(null); @@ -359,11 +420,21 @@ export default function App() { // Right panel collapse/resize state const [rightPanelCollapsed, setRightPanelCollapsed] = useState(false); + // Every collapse/expand routes through here so the pref is persisted once, + // not at each of the three call sites. + const updateRightPanelCollapsed = useCallback( + (collapsed: boolean) => { + setRightPanelCollapsed(collapsed); + persistUiSettings({ right_panel_collapsed: collapsed }); + }, + [persistUiSettings], + ); const [rightPanelWidth, setRightPanelWidth] = useState(320); const rightPanelDragging = useRef(false); const rightPanelStartX = useRef(0); const rightPanelStartWidth = useRef(0); const rightPanelRafId = useRef(0); + const rightPanelLatestWidth = useRef(320); // Groups manifest watching state const [watchedManifestPath, setWatchedManifestPath] = useState(null); @@ -384,7 +455,8 @@ export default function App() { cancelAnimationFrame(rightPanelRafId.current); rightPanelRafId.current = requestAnimationFrame(() => { const delta = rightPanelStartX.current - clientX; - const newWidth = Math.max(200, Math.min(800, rightPanelStartWidth.current + delta)); + const newWidth = clampPanelWidth(rightPanelStartWidth.current + delta); + rightPanelLatestWidth.current = newWidth; setRightPanelWidth(newWidth); }); }; @@ -395,6 +467,7 @@ export default function App() { document.body.style.cursor = ""; document.body.style.userSelect = ""; document.querySelector(".panel-right")?.classList.remove("panel-right-dragging"); + persistUiSettings({ right_panel_width: rightPanelLatestWidth.current }); } }; window.addEventListener("mousemove", onMouseMove); @@ -403,13 +476,14 @@ export default function App() { window.removeEventListener("mousemove", onMouseMove); window.removeEventListener("mouseup", onMouseUp); }; - }, []); + }, [persistUiSettings]); const startRightPanelDrag = useCallback((e: React.MouseEvent) => { e.preventDefault(); rightPanelDragging.current = true; rightPanelStartX.current = e.clientX; rightPanelStartWidth.current = rightPanelWidth; + rightPanelLatestWidth.current = rightPanelWidth; document.body.style.cursor = "col-resize"; document.body.style.userSelect = "none"; document.querySelector(".panel-right")?.classList.add("panel-right-dragging"); @@ -1513,7 +1587,8 @@ export default function App() { annotations_enabled: true, provider: apiProviderDraft, model: selectedApiModel, - api_key_source: "~/.diffcore/config.toml", + api_key_source: llmSettings.global_config_path, + api_key_in_config: true, has_api_key: true, refinement_provider: apiProviderDraft, refinement_model: selectedApiModel, @@ -3649,11 +3724,11 @@ export default function App() { className="btn btn-save-key" disabled={!apiKeyInput.trim()} onClick={handleSaveApiKey} - title="Save API key to ~/.diffcore/config.toml" + title={`Save API key to ${llmSettings.global_config_path}`} > Save - {llmSettings.api_key_source === "~/.diffcore/config.toml" && ( + {llmSettings.api_key_in_config && (