feat(config): XDG base directories and persistent UI preferences - #15
Open
jakob1379 wants to merge 1 commit into
Open
feat(config): XDG base directories and persistent UI preferences#15jakob1379 wants to merge 1 commit into
jakob1379 wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Per-user state was hand-rolled as
$HOME/.diffcore/...in five independent places — global config, refinement cache, comment store, desktop log, embeddings cache — with no XDG support and no Windows fallback. Sincerelease.ymlships Windows MSI/NSIS bundles andHOMEis normally unset there, none of it resolved on Windows at all.Separately, theme and panel layout lived only in localStorage and React state. localStorage is scoped to a browser origin, so running
diffcore-webon a different--portsilently lost the theme; panel width and collapse were lost on every reload.What
pathsmodule resolving config/data/cache/state roots, with all call sites routed through it. XDG variables are honoured as-is on every unix including macOS (~/.config/diffcore, not~/Library/Application Support) — which is why this hand-rolls resolution rather than pulling indirs. Relative$XDG_*_HOMEis ignored per spec. The pureresolve_from()core takes the environment as arguments, so unit tests never touch process env.pr_url's repo clone cache, which landed while this was in progress, moves along with the rest — it was the sixth such path.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; nothing is moved or deleted, so a downgrade still works.
Comments are user-authored review notes, not regenerable data, so they land in
$XDG_DATA_HOMErather than the cache root the spec declares safe to delete.load_cached_comments_filereturns an empty list on any read error, so an unreachable file would have looked like deletion rather than an error.UI preferences persist in
ui.toml, served byget_ui_settings/save_ui_settingsover both the Tauri IPC and the web dispatch. localStorage is kept purely as a synchronous first-paint cache so the theme doesn't flash.ui.tomlis deliberately separate fromconfig.toml. UI prefs are written on every theme toggle and resize;config.tomlholds a plaintext API key and is written rarely. Sharing one file makes each of those writes a read-modify-write of the credential — with the desktop app anddiffcore-webboth running (the configuration this feature exists to support), a theme change could drop a key the other instance just wrote. Keeping them apart removes that interleaving rather than mitigating it.config.tomlis now written atomically (temp + rename) and created0600;fs::writepreviously left the key world-readable.Notable behaviour change
LlmSettingsgainsapi_key_in_config. The settings panel gated the "Clear API key" button onapi_key_sourceequalling 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. Three Playwright cases cover it, one asserting the control survives an unrecognised path.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, pointing at a file that didn't exist yet.Hardening
theme_modeis an enum, so serde rejects unknown values at parse time instead of leaving the frontend to sanitise a free-form stringright_panel_widthclamps on read and write — a hand-edited absurd value rendered an unusable layout, and a negative one failedu32deserialization and took the whole config down with itui.tomldegrades to defaults; preferences must not be able to break startupDrive-by
DIFFCORE_GLOBAL_CONFIG_DIR, a variable nothing reads, and therefore overwrote the developer's real global config with defaults on everycargo testpkgs.clippyandpkgs.rustfmtadded to the devShell; neither was available, socargo clippycould not run at allVerification
cargo test --workspace— 2269 passnpx playwright test— 228 pass, 2 skippednpx tsc --noEmit,npm run build— cleannix build .#diffcore-web— builds; smoke-tested the resulting binary end to end (save prefs, restart on a different port, read back)cargo clippy --all-targets— no new findings vs. baselineMigration paths were verified against a fake
$HOME: legacy config still read, key carried into the new file on first save, old file untouched. Both new regression guards were confirmed to fail against the bug they cover before being kept.Docs live in
docs/file-locations.md; the README section stays short.