fix(tui): prefer codewhale settings path#2516
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the user settings path from the legacy deepseek directory to a new .codewhale directory, while preserving compatibility fallbacks for existing configuration paths. It also updates the documentation and adds tests to verify the path resolution logic. The review feedback highlights two important issues: first, a potential early-return error in Settings::path() if dirs::config_dir() fails in sandboxed environments, which bypasses the primary path resolution; second, a test isolation issue on macOS where dirs::config_dir() ignores the HOME environment variable, potentially overwriting a developer's real settings file during test execution.
| let legacy_config_dir = dirs::config_dir() | ||
| .context("Failed to resolve config directory: not found.")? | ||
| .join("deepseek"); | ||
| Ok(config_dir.join("settings.toml")) | ||
| .join("deepseek") | ||
| .join(SETTINGS_FILE_NAME); | ||
| if legacy_config_dir.exists() { | ||
| return Ok(legacy_config_dir); | ||
| } | ||
|
|
||
| Ok(primary.unwrap_or(legacy_config_dir)) |
There was a problem hiding this comment.
If dirs::config_dir() returns None (which can happen in minimal, sandboxed, or containerized environments), the ? operator will cause Settings::path() to return an error immediately. This prevents the function from falling back to primary (e.g., ~/.codewhale/settings.toml), even if primary is successfully resolved (for instance, via CODEWHALE_HOME).
We should handle dirs::config_dir() as an Option and only return an error if both primary and legacy_config_dir fail to resolve.
let legacy_config_dir = dirs::config_dir()
.map(|d| d.join("deepseek").join(SETTINGS_FILE_NAME));
if let Some(ref path) = legacy_config_dir
&& path.exists()
{
return Ok(path.clone());
}
primary
.or(legacy_config_dir)
.context("Failed to resolve config directory: not found.")| fn settings_path_keeps_platform_config_dir_as_last_legacy_fallback() { | ||
| let _g = config_path_test_guard(); | ||
| let tmp = tempfile::tempdir().expect("tempdir"); | ||
| let _config_override = EnvVarRestore::remove("DEEPSEEK_CONFIG_PATH"); | ||
| let _codewhale_home = EnvVarRestore::set("CODEWHALE_HOME", tmp.path().join(".codewhale")); | ||
| let _home = EnvVarRestore::set("HOME", tmp.path()); | ||
|
|
||
| let config_dir = dirs::config_dir().expect("config dir"); | ||
| let legacy_settings = config_dir.join("deepseek").join("settings.toml"); | ||
| std::fs::create_dir_all(legacy_settings.parent().expect("parent")) | ||
| .expect("legacy config dir"); | ||
| std::fs::write(&legacy_settings, "low_motion = true\n").expect("legacy settings"); | ||
|
|
||
| let got = Settings::path().expect("settings path"); | ||
|
|
||
| assert_eq!(got, legacy_settings); | ||
| } |
There was a problem hiding this comment.
In macOS environments, dirs::config_dir() uses system APIs that ignore the HOME environment variable. As a result, running this test on macOS will write to and overwrite the developer's actual configuration file at ~/Library/Application Support/deepseek/settings.toml.
To prevent tests from modifying the host system's real configuration files, we should sandbox dirs::config_dir() by setting XDG_CONFIG_HOME (for Linux) and APPDATA (for Windows), and skip the file-writing assertion on macOS where sandboxing is not supported via environment variables.
fn settings_path_keeps_platform_config_dir_as_last_legacy_fallback() {
let _g = config_path_test_guard();
let tmp = tempfile::tempdir().expect("tempdir");
let _config_override = EnvVarRestore::remove("DEEPSEEK_CONFIG_PATH");
let _codewhale_home = EnvVarRestore::set("CODEWHALE_HOME", tmp.path().join(".codewhale"));
let _home = EnvVarRestore::set("HOME", tmp.path());
let _xdg_config = EnvVarRestore::set("XDG_CONFIG_HOME", tmp.path());
let _appdata = EnvVarRestore::set("APPDATA", tmp.path());
#[cfg(target_os = "macos")]
{
// On macOS, dirs::config_dir() cannot be sandboxed via env vars,
// so we skip this test to avoid overwriting the developer's real settings.
return;
}
let config_dir = dirs::config_dir().expect("config dir");
let legacy_settings = config_dir.join("deepseek").join("settings.toml");
std::fs::create_dir_all(legacy_settings.parent().expect("parent"))
.expect("legacy config dir");
std::fs::write(&legacy_settings, "low_motion = true\n").expect("legacy settings");
let got = Settings::path().expect("settings path");
assert_eq!(got, legacy_settings);
}![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fn settings_path_keeps_platform_config_dir_as_last_legacy_fallback() { | ||
| let _g = config_path_test_guard(); | ||
| let tmp = tempfile::tempdir().expect("tempdir"); | ||
| let _config_override = EnvVarRestore::remove("DEEPSEEK_CONFIG_PATH"); | ||
| let _codewhale_home = EnvVarRestore::set("CODEWHALE_HOME", tmp.path().join(".codewhale")); | ||
| let _home = EnvVarRestore::set("HOME", tmp.path()); | ||
|
|
||
| let config_dir = dirs::config_dir().expect("config dir"); | ||
| let legacy_settings = config_dir.join("deepseek").join("settings.toml"); | ||
| std::fs::create_dir_all(legacy_settings.parent().expect("parent")) | ||
| .expect("legacy config dir"); | ||
| std::fs::write(&legacy_settings, "low_motion = true\n").expect("legacy settings"); | ||
|
|
||
| let got = Settings::path().expect("settings path"); | ||
|
|
||
| assert_eq!(got, legacy_settings); | ||
| } |
There was a problem hiding this comment.
XDG_CONFIG_HOME not cleared — test may write into the developer's real config dir
dirs::config_dir() on Linux checks XDG_CONFIG_HOME before falling back to $HOME/.config. The test sets HOME to tmp.path() but does not clear XDG_CONFIG_HOME. When that variable is set (common in developer shells and many CI runners), dirs::config_dir() will return $XDG_CONFIG_HOME, which is outside tmp. The test then writes settings.toml to that real path and never cleans it up — either corrupting an existing settings file or leaving a stale file that causes the test to fail on subsequent runs.
Adding EnvVarRestore::remove("XDG_CONFIG_HOME") alongside the existing HOME override is the minimal fix.
|
Thanks @cyq1017 — I harvested this settings-path slice into the v0.8.50 triage branch (#2504) rather than merging the PR directly. Landed commits on #2504:
Local validation:
I left #2369 open because this fixes the settings/TUI prefs path consolidation slice, but not the broader loud migration UX yet. |
|
Thanks @cyq1017 — your contribution landed in
Closing this PR now that the code is on If you want to land more work and would prefer your future PRs merge cleanly without a harvest step, the |
2 participants
Refs #2369
Problem
settings.tomlstill uses the legacy DeepSeek config locations even after the broader config home moved toward.codewhale, which leaves user preferences split across old and new homes.Change
~/.codewhale/settings.tomlfor new settings writes~/.deepseek/settings.tomlas the first legacy read fallbackdeepseek/settings.tomlas the last compatibility fallbackVerification
cargo test -p codewhale-tui settings_path_ --all-features --locked -- --nocapturecargo test -p codewhale-tui settings::tests --all-features --locked -- --nocapturecargo fmt --all -- --checkcargo check -p codewhale-tui --all-features --lockedcargo clippy -p codewhale-tui --all-features --locked -- -D warningsgit diff --check origin/main..HEADGreptile Summary
This PR migrates
Settings::path()from the old~/.config/deepseek/settings.tomllocation to prefer~/.codewhale/settings.tomlfor new installs, while preserving backward-compatibility for users with existing~/.deepseek/settings.tomlor platform config-dir files. The change introduces a dedicatedresolve_settings_path_from_candidateshelper, four new unit tests with anEnvVarRestoreRAII helper, and matching doc/comment updates across the TUI and docs.codewhale_home → legacy_deepseek_home → platform config_dir): existing files are read from their current location; new writes land in~/.codewhale/settings.toml.TuiPrefs::path()(dead code, wired in feat(config): separate tui.toml for theme and keybinds (closes #437) #657) still falls back to~/.deepseek/tui.tomlfor new installs, which will split the config home across~/.codewhale/and~/.deepseek/once that wiring lands.Confidence Score: 5/5
Safe to merge; the three-tier resolver correctly handles new installs, legacy deepseek home, and platform config-dir fallback without data loss.
The path resolution logic is correct and well-tested. The only finding is that TuiPrefs::path() — currently dead code — will write tui.toml to ~/.deepseek/ while settings.toml now goes to ~/.codewhale/, creating a split config home when the TuiPrefs wiring (#657) lands. That inconsistency does not affect live users today.
crates/tui/src/settings.rs — specifically the TuiPrefs::path() fallback and its save() doc comment, which will matter once #657 wires up TuiPrefs.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["Settings::path() called"] --> B{DEEPSEEK_CONFIG_PATH set?} B -- Yes --> C["Return parent(config_path)/settings.toml"] B -- No --> D["codewhale_home() → primary\nlegacy_deepseek_home() → legacy_home\ndirs::config_dir() → legacy_config_dir"] D --> E["resolve_settings_path_from_candidates(...)"] E --> F{primary exists on disk?} F -- Yes --> G["Return ~/.codewhale/settings.toml"] F -- No --> H{legacy_home exists on disk?} H -- Yes --> I["Return ~/.deepseek/settings.toml"] H -- No --> J{legacy_config_dir exists on disk?} J -- Yes --> K["Return config_dir/deepseek/settings.toml"] J -- No --> L{primary is Some?} L -- Yes --> M["Return ~/.codewhale/settings.toml (new write)"] L -- No --> N{legacy_config_dir is Some?} N -- Yes --> O["Return config_dir/deepseek/settings.toml (new write)"] N -- No --> P["Error: no config directory found"]Reviews (2): Last reviewed commit: "fix(tui): isolate settings path fallback..." | Re-trigger Greptile