feat: Move generated app state to XDG state/cache dirs - #407
Conversation
chore: use xdg config path for user config chore: use xdg state path for last session chore: use xdg state path for histories chore: use xdg cache path for streaming cache chore: use xdg config path for runtime client auth config chore: update comments to not explicitly reference `~/.config/spotatui/` chore: show actual config file path in first run prompt
introduce a core state module for machine-managed app state that will move out of config.yml, including volume, shuffle, active source, announcement state, layout sizes, radio stations, and sync token. resolve the state file through the XDG state directory, save it atomically with private file permissions, and add focused round-trip and sanitization coverage.
Separate app-managed runtime state into state.yml while keeping config.yml for user-authored preferences and startup overrides.
Move Spotify OAuth token cache out of the config directory and into the XDG state directory while keeping client.yml as user configuration. Gate cache-dir path helpers behind the streaming feature and document the state/cache locations for history, token cache, and native streaming cache. Persist volume changes through runtime state and flush pending state saves before CLI command exit.
📝 WalkthroughWalkthroughSpotatui separates startup configuration from persisted runtime state, adds XDG-aware config, cache, and state paths, and updates playback, layout, radio, UI, plugin, and documentation code to use the new storage model. ChangesRuntime state migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 6
🧹 Nitpick comments (2)
src/core/plugin_api.rs (1)
423-429: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the runtime-backed snapshot.
Test that changing
RuntimeStateupdates all five migrated fields whileUserConfig.behaviorremains unchanged. The new values are exposed to plugins throughsrc/infra/scripting/engine.rs.As per coding guidelines, behavior changes require adding or adjusting Rust tests.
Also applies to: 474-479
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/plugin_api.rs` around lines 423 - 429, Add Rust regression coverage for config_snapshot, verifying that changing RuntimeState updates all five migrated snapshot fields while UserConfig.behavior remains unchanged. Exercise the plugin-facing values exposed through the scripting engine path, and preserve existing behavior for the unchanged user configuration.Source: Coding guidelines
src/core/user_config.rs (1)
47-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame-named helper with different semantics than
state.rs'ssanitized_radio_stations.This one dedupes by URL;
crate::core::state's module-levelsanitized_radio_stations(used forRuntimeState/state.yml) doesn't. Two functions with the same name and near-identical purpose diverging in behavior is easy to lose track of during future edits.Consider extracting one shared helper (e.g. in
crate::core::state, taking a slice or iterator) and reusing it from bothuser_config.rsandstate.rs, so both call sites get the same trim/empty-filter/dedupe guarantees for free.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/user_config.rs` around lines 47 - 64, Consolidate the duplicated sanitized_radio_stations helpers by defining one shared implementation in crate::core::state and reusing it from both user_config.rs and state.rs. Ensure the shared helper consistently trims names and URLs, filters empty values, and deduplicates stations by URL, while preserving both callers’ existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/plugins/README.md`:
- Around line 24-35: Make XDG path handling consistent across all listed sites:
in examples/plugins/README.md lines 24-35 and the install commands in
examples/plugins/accent-cycler.lua lines 6-10,
examples/plugins/now-playing-webhook.lua lines 7-9, and
examples/plugins/queue-browser.lua lines 6-10, normalize relative
XDG_CONFIG_HOME values to the application fallback before creating or copying
files; document in docs/native-streaming.md lines 29-37 that both
XDG_CONFIG_HOME and XDG_CACHE_HOME must be absolute, and in docs/scripting.md
lines 9-10 and docs/themes.md lines 24-26 that XDG_CONFIG_HOME is honored only
when absolute.
In `@README.md`:
- Around line 222-230: Update the XDG documentation and installation examples to
state that XDG_CONFIG_HOME, XDG_STATE_HOME, and XDG_CACHE_HOME are used only
when set to absolute paths; unset or relative values must use the HOME-based
fallback. In README.md (222-230 and 329-336), PLUGINS.md (20-23),
docs/configuration.md (14-14), and docs/keybindings.md (40-41), revise the prose
and config_home migration calculation accordingly. In
examples/plugins/session-stats/main.lua (7-9),
examples/plugins/track-info-popup.lua (4-6), and
examples/plugins/track-notifier.lua (4-6), validate XDG_CONFIG_HOME as absolute
before using it for copying, otherwise use the fallback path.
In `@src/infra/queue/dispatch.rs`:
- Around line 95-98: Persist runtime state after each ChangeVolume handler
updates RuntimeState: add the established save operation in
src/infra/queue/dispatch.rs lines 95-98, src/infra/subsonic/dispatch.rs lines
120-125, and src/infra/youtube/dispatch.rs lines 122-126, while preserving the
existing volume updates and return behavior.
In `@src/infra/radio/dispatch.rs`:
- Around line 89-93: Update the IoEvent::ChangeVolume branch to call
schedule_state_save() after assigning runtime_state.volume_percent, ensuring the
changed volume is queued for persistence before returning true.
In `@src/runtime.rs`:
- Around line 1111-1129: Gate the compatibility assignments for volume_percent,
sidebar_width_percent, playbar_height_rows, and library_height_percent behind
should_save_initial_state so they run only during initial state creation.
Preserve values loaded from state.yml on subsequent launches, and keep saving
the resulting migrated runtime state through the existing state_path persistence
block.
In `@src/tui/handlers/resize.rs`:
- Around line 67-86: Update reset_layout to clamp the config-derived
playbar_height_rows value to MAX_PLAYBAR_ROWS, matching the limit enforced by
increase_playbar_height. Preserve the existing default fallback and state-save
behavior.
---
Nitpick comments:
In `@src/core/plugin_api.rs`:
- Around line 423-429: Add Rust regression coverage for config_snapshot,
verifying that changing RuntimeState updates all five migrated snapshot fields
while UserConfig.behavior remains unchanged. Exercise the plugin-facing values
exposed through the scripting engine path, and preserve existing behavior for
the unchanged user configuration.
In `@src/core/user_config.rs`:
- Around line 47-64: Consolidate the duplicated sanitized_radio_stations helpers
by defining one shared implementation in crate::core::state and reusing it from
both user_config.rs and state.rs. Ensure the shared helper consistently trims
names and URLs, filters empty values, and deduplicates stations by URL, while
preserving both callers’ existing behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b4de3df-1b0b-4452-bc35-d12b5db9aaf5
📒 Files selected for processing (53)
PLUGINS.mdREADME.mddocs/configuration.mddocs/keybindings.mddocs/native-streaming.mddocs/scripting.mddocs/themes.mdexamples/plugins/README.mdexamples/plugins/accent-cycler.luaexamples/plugins/now-playing-webhook.luaexamples/plugins/queue-browser.luaexamples/plugins/session-stats/main.luaexamples/plugins/track-info-popup.luaexamples/plugins/track-notifier.luasrc/cli/plugin.rssrc/core/app.rssrc/core/config.rssrc/core/first_run.rssrc/core/layout.rssrc/core/mod.rssrc/core/paths.rssrc/core/persisted_playback.rssrc/core/plugin_api.rssrc/core/state.rssrc/core/user_config.rssrc/infra/history.rssrc/infra/local/dispatch.rssrc/infra/media_metadata.rssrc/infra/network/native_shuffle.rssrc/infra/network/playback.rssrc/infra/network/utils.rssrc/infra/player/events.rssrc/infra/player/streaming.rssrc/infra/queue/dispatch.rssrc/infra/radio/dispatch.rssrc/infra/radio/mod.rssrc/infra/scripting/engine.rssrc/infra/subsonic/dispatch.rssrc/infra/youtube/dispatch.rssrc/infra/youtube/playlists.rssrc/runtime.rssrc/tui/handlers/announcement_prompt.rssrc/tui/handlers/library.rssrc/tui/handlers/mod.rssrc/tui/handlers/mouse.rssrc/tui/handlers/playlist.rssrc/tui/handlers/resize.rssrc/tui/handlers/search_results.rssrc/tui/handlers/select_device.rssrc/tui/runner.rssrc/tui/ui/library.rssrc/tui/ui/lyrics.rssrc/tui/ui/player.rs
DinoLeung
changed the title
Document that XDG directory variables are honored only when set to absolute paths, with HOME-based fallbacks for unset or relative values. Update plugin install and migration snippets to mirror runtime path resolution.
Persist decoded-backend volume changes through scheduled runtime state saves. Apply configured volume and layout defaults only when the corresponding runtime state fields are missing, so saved state.yml values remain authoritative after startup. Clamp config-derived playbar height during layout reset and document the initial-default behavior.
Add regression coverage for config_snapshot and the Lua-facing spotatui.config() path to ensure migrated behavior fields come from RuntimeState without mutating UserConfig. Share radio station sanitization between runtime state and user config.
There was a problem hiding this comment.
- Free-source users get pushed into a forced Spotify OAuth flow on upgrade (inline on
src/runtime.rs). - Listening history is silently orphaned, along with
last_session.yml(inline onsrc/infra/history.rs). - The window for doing this migration later closes on the first config save (inline on
src/core/user_config.rs).
All three are covered by one startup shim, run before state::load:
- If
<state>/state.ymlis absent, deserialize the legacy keys off the rawconfig.ymlinto a smallLegacyRuntimeKeysstruct and seedPersistedRuntimeState(active_source,shuffle_enabled,seen_announcement_ids, plus the app-savedradio_stations). - For
.spotify_token_cache.json,last_session.yml,history/, andstreaming_cache/: if the new path is absent and the old one exists,fs::renameit, falling back to copy across filesystems.
Finding 3 is why this belongs in this PR rather than a follow-up: once save_config() runs once, the legacy keys are gone for good.
Non-blocking
Four more inline: an existing-user regression on in-app saved radio stations, blind-overwrite vs the read-modify-write that save_config does, state-dir permissions, and a Windows path separator nit. Plus:
- Two dedupe implementations for radio stations (
state::sanitized_radio_stationsuses aHashSet,radio::dispatch::merged_radio_stationsre-implements it with aVecscan). Tiny lists, so this is drift risk rather than perf. dismissed_announcementshas no reader onmaineither. Good moment to drop dead state rather than carry it into a new file format.- Volume now persists from three layers (
App::{increase,decrease,set}_volume, each decoded dispatcher, and nowNetwork::change_volume). Correct, just redundant. - No CHANGELOG entry, against a repo that keeps detailed prose entries for user-visible changes.
Thanks for taking this on, the config/state separation is overdue.
| // tolerates its absence; the user can add Spotify later via in-TUI login. | ||
| let spotify_required = matches.subcommand_name().is_some() | ||
| || user_config.behavior.active_source == crate::core::source::Source::Spotify; | ||
| || runtime_state.active_source == crate::core::source::Source::Spotify; |
There was a problem hiding this comment.
Blocking: this reverts every free-source user to Spotify on upgrade, and then hard-blocks startup.
BehaviorConfigString::active_source was deleted and there is no deny_unknown_fields, so an existing config.yml with active_source: Local is silently ignored. No state.yml exists yet, so RuntimeState::default().active_source is Source::Spotify, which makes spotify_required true here.
That routes into authenticate_with_fallback -> ensure_auth_token(interactive: true), and since the token cache also moved there is no cached token, so src/core/auth.rs:344 opens a browser and blocks on the callback server. The first-run picker cannot rescue it either: it bails when client.yml exists, and apply_selections already wrote one via init_default_spotify_config().
Net effect: someone deliberately running spotatui without Spotify cannot start the app after upgrading.
| .join(LISTENS_FILE_NAME), | ||
| ) | ||
| crate::core::paths::app_state_dir() | ||
| .map(|dir| dir.join(HISTORY_SUBDIR).join(LISTENS_FILE_NAME)) |
There was a problem hiding this comment.
Blocking: this orphans accumulated user data with no migration.
history/listens.jsonl, last_synced_at.txt, and last_recap_at.txt all move from ~/.config/spotatui/history/ to ~/.local/state/spotatui/history/. That is the data behind spotatui history recap, stats, and streaks. After upgrade it reads as empty and the user gets no indication where it went. The PR body flags the token cache but not this one, and losing history feels considerably worse than a re-auth.
src/core/persisted_playback.rs:117 has the same shape: last_session.yml moves, so resume-last-song quietly stops working once.
Both are covered by the rename shim described in the review summary.
| subsonic_url: self.behavior.subsonic_url.clone(), | ||
| subsonic_username: self.behavior.subsonic_username.clone(), | ||
| subsonic_password: self.behavior.subsonic_password.clone(), | ||
| ytdlp_path: self.behavior.ytdlp_path.clone(), |
There was a problem hiding this comment.
Blocking, and the reason findings 1 and 2 cannot be deferred to a follow-up PR.
build_behavior reconstructs BehaviorConfigString wholesale from self.behavior. Since active_source, shuffle_enabled, seen_announcement_ids, and dismissed_announcements were removed from that struct, the first save_config() after upgrade rewrites the user's config.yml without them, permanently.
So a one-time seed added in a later release would find nothing left to read. The migration has to land in the same release that moves the keys.
| app.set_status_message("Radio station has no stream URL".to_string(), 4); | ||
| return; | ||
| }; | ||
| if app.is_configured_radio_station_url(url) { |
There was a problem hiding this comment.
Existing-user regression, worth handling in the same shim.
Before this PR, F (favorite) wrote into behavior.radio_stations in config.yml. After it, everything in config.yml counts as configured, so this guard fires and refuses removal with "Radio station is configured in config.yml". Every station a user saved in-app now needs a manual file edit to remove.
The ownership model itself is good, and I like that the guard exists rather than failing silently. The issue is only the day-one transition: moving pre-existing config.yml stations into state.yml during migration would avoid it entirely.
| } | ||
|
|
||
| /// Save state atomically and privately. | ||
| pub fn save(path: &Path, state: &PersistedRuntimeState) -> Result<()> { |
There was a problem hiding this comment.
Non-blocking, but worth a deliberate call: this is a blind overwrite, where save_config does a read-modify-write (user_config.rs:2205 reads the existing file and merges before writing).
Two concurrent spotatui instances will now clobber each other's volume, saved stations, and seen announcements, last writer wins. The merge previously made that mostly harmless. Probably acceptable for machine-owned state, but it is a behavior change that falls out of the refactor rather than an explicit decision.
| ) { | ||
| (Some(app_config_dir), Some(app_state_dir)) => { | ||
| fs::create_dir_all(&app_config_dir)?; | ||
| fs::create_dir_all(&app_state_dir)?; |
There was a problem hiding this comment.
Non-blocking: the state dir is created here at umask mode (typically 0755) and the OAuth token cache lands in it, but only state::save chmods it to 0700, and only on the first state write.
write_private_file still puts 0600 on the token file, so real exposure is limited to directory listing. Calling ensure_private_state_dir here instead of a bare create_dir_all would close it.
Related, and not a regression: the config dir still gets a .gitignore and the state dir gets none. The token cache was never in the old gitignore either, so this is a pre-existing gap that moved rather than a new one. Dropping the panic-log and streaming-cache entries is correct now that neither lives there.
| const FALLBACK_CONFIG_DIR: &str = ".config"; | ||
| #[cfg(any(feature = "streaming", test))] | ||
| const FALLBACK_CACHE_DIR: &str = ".cache"; | ||
| const FALLBACK_STATE_DIR: &str = ".local/state"; |
There was a problem hiding this comment.
Nit: this is one joined component with an embedded separator, so on Windows PathBuf::join yields C:\Users\x\.local/state\spotatui. Windows APIs accept it so nothing breaks, but .join(".local").join("state") at the call site is cleaner and matches how FALLBACK_CONFIG_DIR and FALLBACK_CACHE_DIR are each a single component.
| } | ||
| } | ||
|
|
||
| fn merged_radio_stations( |
There was a problem hiding this comment.
Nit: this duplicates state::sanitized_radio_stations, which applies the same trim / skip-blank / dedupe-by-URL rule using a HashSet rather than a Vec linear scan. Station lists are tiny so the perf difference is irrelevant, but two implementations of one rule can drift.
Since the merge here also needs to preserve configured-then-saved ordering, one option is to call the shared sanitizer over the chained iterator instead.
|
Thanks for the thorough review, and apologies for an extra long PR. I did want to keep it short and focus on the state and cache decoupling, it quickly gets out of hand. Will be addressing them later in the week. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/state.rs (1)
337-347: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBoth atomic-write helpers derive the temporary file name from the target path only. A fixed
.yml.tmpsibling is not unique per writer, so two processes write the same temporary file and then both rename it over the target. The published file can hold interleaved bytes.
src/core/state.rs#L337-L347: make thestate.yml.tmpname unique per writer, or usetempfile::NamedTempFile::new_inpluspersist.src/core/migrations.rs#L383-L388: apply the same change to theconfig.yml.tmpwrite, and extract one sharedwrite_atomic_privatehelper used by both call sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/state.rs` around lines 337 - 347, The atomic write logic in write_state and the corresponding migrations write path uses a shared fixed temporary filename, allowing concurrent writers to collide. Extract a shared write_atomic_private helper, using a unique per-writer temporary file (such as tempfile::NamedTempFile::new_in with persist), and update both call sites to use it: src/core/state.rs lines 337-347 and src/core/migrations.rs lines 383-388. Preserve private-file permissions and atomic replacement of the target.src/core/app.rs (1)
4042-4054: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
flush_state_savedrops a failed save permanently instead of retrying it.
flush_state_saveclearsstate_save_duetoNonebefore attempting the write. On failure, it merges the patch back intopending_state_save_patch, butstate_save_duestaysNone. The next call, including a forced call at shutdown, returns immediately atlet Some(due) = self.state_save_due else { return; };and never retries the write. A single transient disk error silently discards the pending volume, shuffle, or radio-station patch for the rest of the session.Re-arm
state_save_dueon failure so a subsequent flush, including the forced one at exit, retries the write.🛠️ Proposed fix to re-arm the retry on failure
pub fn flush_state_save(&mut self, force: bool) { let Some(due) = self.state_save_due else { return; }; if force || Instant::now() >= due { let patch = std::mem::take(&mut self.pending_state_save_patch); - self.state_save_due = None; if let Err(e) = self.save_runtime_state(&patch) { self.pending_state_save_patch.merge_patch(&patch); + self.state_save_due = Some(Instant::now()); self.handle_error(anyhow!("Failed to save state: {}", e)); + } else { + self.state_save_due = None; } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/app.rs` around lines 4042 - 4054, Update flush_state_save so a failed save re-arms state_save_due after merging the patch back into pending_state_save_patch, allowing subsequent and forced flushes to retry. Preserve the existing due-time check and successful-save behavior.
🧹 Nitpick comments (5)
src/core/migrations.rs (1)
548-569: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the copy fallback path.
The tests exercise
fs::renameonly.copy_legacy_path_if_unclaimed,copy_legacy_dir_if_unclaimed, andcopy_dir_contentsnever run, so the nested-directory copy, the cleanup on copy failure, and the resulting permissions are unverified. That code only triggers on a cross-device move, which is exactly the case a user hits when$XDG_STATE_HOMEis on another mount.Call
copy_legacy_dir_if_unclaimeddirectly with a syntheticstd::io::Errorto cover it without a second filesystem.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/migrations.rs` around lines 548 - 569, Add a test beside legacy_path_migration_moves_directory_when_target_is_empty that directly invokes copy_legacy_dir_if_unclaimed with a synthetic std::io::Error, exercising nested-directory copying, source cleanup after copy failure, and resulting permissions without requiring a cross-device filesystem.Source: Coding guidelines
src/core/paths.rs (1)
56-64: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCreate the directory with mode 0700 instead of chmod after creation.
create_dir_alluses the process umask, thenset_permissionsnarrows the mode. Between those two calls the directory is group/world readable on a permissive umask. This directory holds OAuth tokens and streaming credentials, so the window matters.
std::os::unix::fs::DirBuilderExt::modesets the mode at creation time. Keep theset_permissionscall so an already existing directory with wide permissions is still tightened.🔒 Proposed change
pub(crate) fn ensure_private_dir(dir: &Path) -> Result<()> { - std::fs::create_dir_all(dir).with_context(|| format!("creating {}", dir.display()))?; + #[cfg(unix)] + { + use std::os::unix::fs::DirBuilderExt; + std::fs::DirBuilder::new() + .recursive(true) + .mode(0o700) + .create(dir) + .with_context(|| format!("creating {}", dir.display()))?; + } + #[cfg(not(unix))] + std::fs::create_dir_all(dir).with_context(|| format!("creating {}", dir.display()))?; #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; std::fs::set_permissions(dir, std::fs::Permissions::from_mode(0o700)) .with_context(|| format!("setting private permissions on {}", dir.display()))?; } Ok(()) }Note: with
recursive(true), the mode applies to created parent components too. If a parent such as~/.local/statemust stay at 0755, split the parent creation from the final component.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/paths.rs` around lines 56 - 64, Update ensure_private_dir to create directories through a Unix DirBuilder configured with recursive creation and mode 0o700, so newly created components receive the private mode immediately. Retain the existing set_permissions call to tighten permissions on directories that already exist, while preserving the current error-context behavior.src/core/state.rs (2)
416-429: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
seen_announcement_idsanddismissed_announcementsgrow without bound.The merge only unions, so an ID is never dropped. The lists grow with every announcement the feed has ever published, and the whole list is rewritten on each save. This is small per entry, so it is not urgent, but there is no upper bound.
Consider a cap, for example keep the most recent N IDs, or prune IDs that are no longer present in the announcement feed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/state.rs` around lines 416 - 429, Update merged_ids to enforce a bounded ID history after merging existing and incoming values, retaining the most recent N entries according to the established ordering; preserve trimming and duplicate/empty filtering, and define or reuse a shared cap so both seen_announcement_ids and dismissed_announcements cannot grow without bound.
369-405: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd an explicit pre/postcondition to
merge_state_patch.
merge_state_patchunionsradio_stations,seen_announcement_ids, anddismissed_announcements, so anAnnouncements(_, _)patch can re-add dismissed or seen IDs. Sinceremove_radio_station_by_urlmust usesave_removing_radio_stationinstead ofsave, spell that rule next tomerge_state_patchso the deletion contract is not hidden inside callers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/state.rs` around lines 369 - 405, Document an explicit precondition and postcondition next to merge_state_patch: callers must not use it for Announcements(_, _) patches that could re-add seen or dismissed IDs, and radio-station deletion must use save_removing_radio_station rather than save. Ensure remove_radio_station_by_url follows this deletion path and preserve the existing union behavior for non-deletion merges.src/core/app.rs (1)
4125-4126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
App::persist_volume/App::persist_shufflehelpers to remove nine duplicated persistence snippets. Every site repeats the same two-line pattern: set aruntime_statefield, then callschedule_state_savewith the matchingPersistedRuntimeStateconstructor. One pair of helper methods onAppremoves all of it.
src/core/app.rs#L4125-L4126: replace withself.persist_volume(next_volume);.src/core/app.rs#L4140-L4141: replace withself.persist_volume(next_volume);.src/core/app.rs#L4181-L4182: replace withself.persist_volume(next_volume);.src/core/app.rs#L4196-L4197: replace withself.persist_volume(next_volume);.src/core/app.rs#L4242-L4243: replace withself.persist_volume(next_volume_u8);.src/core/app.rs#L4257-L4258: replace withself.persist_volume(next_volume_u8);.src/core/app.rs#L6337-L6338: replace withself.persist_shuffle(new_shuffle_state);.src/runtime.rs#L2353-L2356: replace withapp_lock.persist_shuffle(shuffle);(add apubvisibility on the helper soruntime.rscan call it, or expose a thinpubwrapper).src/runtime.rs#L2366-L2369: replace withapp_lock.persist_shuffle(shuffle);.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/app.rs` around lines 4125 - 4126, The persistence logic is duplicated across volume and shuffle update sites. Add App::persist_volume and App::persist_shuffle helpers that update the corresponding runtime_state field and call schedule_state_save with the matching PersistedRuntimeState constructor, then replace the seven listed sites in src/core/app.rs with the appropriate helper calls and both listed sites in src/runtime.rs with app_lock.persist_shuffle(shuffle); make the helpers public or provide a public wrapper for runtime.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 7: Update the CHANGELOG entry to state that legacy native streaming
credentials and audio cache may be migrated automatically when their new cache
locations are empty, instead of claiming native streaming caches are never
migrated. Preserve the existing note that Spotify token caches are not migrated
automatically.
In `@src/core/migrations.rs`:
- Around line 193-219: Update copy_dir_contents to detect symlinks with
entry.file_type().is_symlink() before directory recursion and skip or safely
copy them without following links, preventing recursive self-copying. Also apply
the required permission fix wherever migration creates directories, including
the top-level fs::create_dir(state_path) path and the nested
fs::create_dir(&target) path.
In `@src/infra/player/streaming.rs`:
- Line 759: Propagate all ensure_private_dir failures instead of discarding
them: update the credential-cache initialization at
src/infra/player/streaming.rs:759 to use ?, and update the persistence flow at
src/infra/player/streaming.rs:1291 to propagate both directory-setup and write
errors, returning an ID only after persistence succeeds.
---
Outside diff comments:
In `@src/core/app.rs`:
- Around line 4042-4054: Update flush_state_save so a failed save re-arms
state_save_due after merging the patch back into pending_state_save_patch,
allowing subsequent and forced flushes to retry. Preserve the existing due-time
check and successful-save behavior.
In `@src/core/state.rs`:
- Around line 337-347: The atomic write logic in write_state and the
corresponding migrations write path uses a shared fixed temporary filename,
allowing concurrent writers to collide. Extract a shared write_atomic_private
helper, using a unique per-writer temporary file (such as
tempfile::NamedTempFile::new_in with persist), and update both call sites to use
it: src/core/state.rs lines 337-347 and src/core/migrations.rs lines 383-388.
Preserve private-file permissions and atomic replacement of the target.
---
Nitpick comments:
In `@src/core/app.rs`:
- Around line 4125-4126: The persistence logic is duplicated across volume and
shuffle update sites. Add App::persist_volume and App::persist_shuffle helpers
that update the corresponding runtime_state field and call schedule_state_save
with the matching PersistedRuntimeState constructor, then replace the seven
listed sites in src/core/app.rs with the appropriate helper calls and both
listed sites in src/runtime.rs with app_lock.persist_shuffle(shuffle); make the
helpers public or provide a public wrapper for runtime.rs.
In `@src/core/migrations.rs`:
- Around line 548-569: Add a test beside
legacy_path_migration_moves_directory_when_target_is_empty that directly invokes
copy_legacy_dir_if_unclaimed with a synthetic std::io::Error, exercising
nested-directory copying, source cleanup after copy failure, and resulting
permissions without requiring a cross-device filesystem.
In `@src/core/paths.rs`:
- Around line 56-64: Update ensure_private_dir to create directories through a
Unix DirBuilder configured with recursive creation and mode 0o700, so newly
created components receive the private mode immediately. Retain the existing
set_permissions call to tighten permissions on directories that already exist,
while preserving the current error-context behavior.
In `@src/core/state.rs`:
- Around line 416-429: Update merged_ids to enforce a bounded ID history after
merging existing and incoming values, retaining the most recent N entries
according to the established ordering; preserve trimming and duplicate/empty
filtering, and define or reuse a shared cap so both seen_announcement_ids and
dismissed_announcements cannot grow without bound.
- Around line 369-405: Document an explicit precondition and postcondition next
to merge_state_patch: callers must not use it for Announcements(_, _) patches
that could re-add seen or dismissed IDs, and radio-station deletion must use
save_removing_radio_station rather than save. Ensure remove_radio_station_by_url
follows this deletion path and preserve the existing union behavior for
non-deletion merges.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a025616-0d2a-4074-a708-5f2fe4f0beec
📒 Files selected for processing (26)
CHANGELOG.mdsrc/core/app.rssrc/core/config.rssrc/core/first_run.rssrc/core/layout.rssrc/core/migrations.rssrc/core/mod.rssrc/core/paths.rssrc/core/persisted_playback.rssrc/core/state.rssrc/infra/history.rssrc/infra/local/dispatch.rssrc/infra/network/native_shuffle.rssrc/infra/network/playback.rssrc/infra/player/events.rssrc/infra/player/streaming.rssrc/infra/queue/dispatch.rssrc/infra/radio/dispatch.rssrc/infra/subsonic/dispatch.rssrc/infra/youtube/dispatch.rssrc/runtime.rssrc/tui/handlers/announcement_prompt.rssrc/tui/handlers/library.rssrc/tui/handlers/resize.rssrc/tui/handlers/select_device.rssrc/tui/runner.rs
🚧 Files skipped from review as they are similar to previous changes (18)
- src/core/mod.rs
- src/tui/handlers/select_device.rs
- src/tui/handlers/announcement_prompt.rs
- src/tui/handlers/library.rs
- src/infra/player/events.rs
- src/tui/runner.rs
- src/infra/queue/dispatch.rs
- src/infra/subsonic/dispatch.rs
- src/core/config.rs
- src/infra/history.rs
- src/tui/handlers/resize.rs
- src/core/first_run.rs
- src/infra/network/playback.rs
- src/infra/local/dispatch.rs
- src/infra/youtube/dispatch.rs
- src/core/layout.rs
- src/core/persisted_playback.rs
- src/infra/network/native_shuffle.rs
|
|
||
| ### Changed | ||
|
|
||
| - **Generated app state now uses XDG state/cache directories**: `config.yml` stays in the app config directory for user-authored settings, while runtime-managed state (`state.yml`), listening history, last playback session, and Spotify token caches move to the app state directory. Native streaming credentials and audio cache move to the app cache directory. Existing config-dir runtime fields, radio favorites, listening history, and playback-session files are migrated on first use when the new path is still empty, preserving free-source startup and existing in-app radio favorites during upgrade. Existing Spotify token caches and native streaming caches are not migrated automatically. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Determine whether the streaming cache path goes through the migration helper
rg -n -B6 -A10 'get_default_cache_path' src/infra/player/streaming.rs
rg -n 'migrate_legacy_path_if_unclaimed|state_file_path_with_legacy_config_rename' --type=rustRepository: LargeModGames/spotatui
Length of output: 2197
🏁 Script executed:
#!/bin/bash
set -u
echo "== Rust migration symbols =="
rg -n 'migrate_legacy_path_if_unclaimed|state_file_path_with_legacy_config_rename|config/streaming_cache|cache/streaming_cache|streaming_cache' --type=rust || true
echo
echo "== File outlines for likely path/migration files =="
fd -e rs 'paths|migrations' src || true
echo
echo "== Relevant source snippets =="
for f in $(fd -e rs 'paths|migrations|streaming' src); do
echo "--- $f"
rg -n -B4 -A12 'migrate_legacy_path_if_unclaimed|state_file_path_with_legacy_config_rename|config/streaming_cache|cache/streaming_cache|streaming_cache|get_default_cache_path|app_cache_dir' "$f" || true
doneRepository: LargeModGames/spotatui
Length of output: 397
🏁 Script executed:
#!/bin/bash
set -u
echo "== Candidate Rust files =="
git ls-files '*.rs' | sed -n '1,120p'
echo
echo "== Search all tracked Rust files for symbols =="
python3 - <<'PY'
import subprocess, pathlib, re
files = subprocess.check_output(['git','ls-files','*.rs'], text=True).splitlines()
patterns = [
'migrate_legacy_path_if_unclaimed',
'state_file_path_with_legacy_config_rename',
'config/streaming_cache',
'cache/streaming_cache',
'streaming_cache',
'get_default_cache_path',
'app_cache_dir',
'ensure_private_dir',
]
for pat in patterns:
print(f'\n-- {pat} --')
for path in files:
try:
text = pathlib.Path(path).read_text(errors='replace')
except Exception:
continue
for i, line in enumerate(text.splitlines(), 1):
if pat in line:
start=max(1,i-6); end=min(len(text.splitlines()), i+10)
lines=text.splitlines()
print(f'[{path}:{i}]')
for j in range(start,end+1):
if j <= len(lines):
print(f' {j}: {lines[j-1]}')
print()
PY
echo
echo "== CHANGELOG snippet =="
sed -n '1,40p' CHANGELOG.md 2>/dev/null || true
echo
echo "== Migration/test symbols in tests only =="
find . -path '*/tests/*' -name '*.rs' -o -path '*/tests/*' -name '*.toml' | sort | sed -n '1,160p'Repository: LargeModGames/spotatui
Length of output: 37142
🏁 Script executed:
#!/bin/bash
set -u
echo "== Relevant migration helper implementation =="
sed -n '30,120p' src/core/migrations.rs
echo
echo "== Migration callers across Rust =="
rg -n 'state_file_path_with_legacy_config_rename\(|state_file_path_with_legacy_config_rename\(\s*$|get_default_cache_path\(\)|new_cache_only|resolve_streaming_credentials|ensure_streaming_credentials_cached|Cache::new' --type=rust
echo
echo "== Full migration test case =="
sed -n '540,568p' src/core/migrations.rsRepository: LargeModGames/spotatui
Length of output: 3151
Correct the native streaming cache migration claim.
This entry says native streaming caches are not migrated automatically, but legacy_path_migration_moves_directory_when_target_is_empty migrates config/streaming_cache to <cache>/streaming_cache when the cache target is empty. Tell users that legacy native streaming credentials and audio cache can migrate; otherwise they may expect a re-login that does not happen.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` at line 7, Update the CHANGELOG entry to state that legacy
native streaming credentials and audio cache may be migrated automatically when
their new cache locations are empty, instead of claiming native streaming caches
are never migrated. Preserve the existing note that Spotify token caches are not
migrated automatically.
| fn copy_dir_contents(legacy_path: &Path, state_path: &Path) -> Result<()> { | ||
| for entry in | ||
| fs::read_dir(legacy_path).with_context(|| format!("reading {}", legacy_path.display()))? | ||
| { | ||
| let entry = entry?; | ||
| let source = entry.path(); | ||
| let target = state_path.join(entry.file_name()); | ||
| let metadata = entry | ||
| .metadata() | ||
| .with_context(|| format!("checking {}", source.display()))?; | ||
|
|
||
| if metadata.is_dir() { | ||
| fs::create_dir(&target).with_context(|| format!("creating {}", target.display()))?; | ||
| copy_dir_contents(&source, &target)?; | ||
| } else { | ||
| fs::copy(&source, &target).with_context(|| { | ||
| format!( | ||
| "copying legacy app data from {} to {}", | ||
| source.display(), | ||
| target.display() | ||
| ) | ||
| })?; | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check which directories reach copy_legacy_dir_if_unclaimed and whether any hold credentials
rg -n -B4 -A4 'migrate_legacy_path_if_unclaimed|state_file_path_with_legacy_config_rename' --type=rust
rg -n -B4 -A6 'streaming_cache|credentials' src/infra/player/streaming.rsRepository: LargeModGames/spotatui
Length of output: 160
🌐 Web query:
Rust std fs DirEntry metadata follows symlinks file_type does not
💡 Result:
Contrary to the premise of your query, both std::fs::DirEntry::metadata and std::fs::DirEntry::file_type do not follow symbolic links [1][2]. Official Rust documentation explicitly states that for a DirEntry instance, the metadata method "will not traverse symlinks if this entry points at a symlink" [1]. Similarly, the documentation for file_type explicitly states that "this function will not traverse symlinks if this entry points at a symlink" [2]. If you need to follow symbolic links to obtain the metadata or file type of the target, you must use std::fs::metadata or std::fs::File::metadata instead [1][2]. The confusion may arise from platform-specific behaviors where implementations vary. For example, on some Unix platforms, the underlying implementation of DirEntry methods might perform a lstat call (which does not follow symlinks) [1][3]. Conversely, if you use the standalone fs::metadata function (rather than the DirEntry method), it is designed to follow symbolic links, which is often where users encounter behavior that differs from what they expect when working with directory entries [4][5].
Citations:
- 1: https://doc.rust-lang.org/stable/std/fs/struct.DirEntry.html
- 2: https://doc.rust-lang.org/nightly/std/fs/struct.DirEntry.html
- 3: https://github.com/rust-lang/rust/blob/29ea4eef9fa6e36f40bc1f31eb1e56bf5941ee72/src/libstd/sys/unix/fs.rs
- 4: https://stackoverflow.com/questions/65752474/check-if-a-file-is-a-symlink-in-rust-2018-on-windows
- 5: https://doc.rust-lang.org/stable/std/fs/struct.FileType.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching migrations.rs:"
fd -a 'migrations\.rs$' . || true
echo
echo "Read relevant migrations.rs sections:"
if [ -f src/core/migrations.rs ]; then
wc -l src/core/migrations.rs
sed -n '140,230p' src/core/migrations.rs | cat -n -v
fi
echo
echo "Search migration symbols:"
rg -n -B3 -A6 'copy_dir_contents|copy_legacy|legacy_path|state_file_path_with_legacy_config_rename|migrate_legacy_path_if_unclaimed|streaming_cache|credentials' src || true
echo
echo "Platform/path/permission helper candidates:"
fd -a 'paths.*\.rs$|.*paths.*\.rs$' src || true
rg -n 'ensure_private_dir|mode|PermissionsExt|chmod|streaming_cache' src/core src || trueRepository: LargeModGames/spotatui
Length of output: 50378
🌐 Web query:
docs.rs Rust std::fs::DirEntry::metadata follows symbolic links filesystem DirEntry file_type is_symlink
💡 Result:
In Rust, the std::fs::DirEntry::metadata method does not follow symbolic links; it queries the metadata of the symbolic link itself if the entry points to one [1][2]. Similarly, the std::fs::DirEntry::file_type method returns the file type of the entry without traversing symbolic links [2][3]. When using these methods on a DirEntry: 1. Metadata: The metadata method explicitly states that it will not traverse symlinks if the entry points at a symlink [1][4]. If you require the metadata of the target file instead, you must use std::fs::metadata or std::fs::File::metadata on the path, which do follow symbolic links [1][2]. 2. File Type: The file_type method also does not traverse symlinks [2]. Consequently, if a DirEntry represents a symbolic link, its file type will correctly identify it as a link (i.e., its is_symlink method will return true) [5][2]. Platform-specific behavior: On Unix platforms, the DirEntry::metadata method is often implemented as the equivalent of calling symlink_metadata on the path [1][4]. While some Unix platforms might require an additional call to retrieve the underlying file type, the method ensures that the symbolic link itself is examined rather than its target [2][4].
Citations:
- 1: https://doc.rust-lang.org/std/fs/struct.DirEntry.html
- 2: https://doc.rust-lang.org/nightly/std/fs/struct.DirEntry.html
- 3: https://doc.rust-lang.org/1.68.2/std/fs/struct.DirEntry.html
- 4: https://doc.rust-lang.org/1.74.0/std/fs/struct.DirEntry.html
- 5: https://doc.rust-lang.org/stable/std/fs/struct.FileType.html
Fix the recursive symlink copy in directory migration.
entry.metadata() does not follow symlinks, but when it identifies a symlink target that is a directory, fs::copy copies the link instead of skipping it. A symlink pointing back into the legacy directory can be copied into itself, causing unbounded recursion and stack overflow. Use entry.file_type().is_symlink() to skip or copy symlink entries before recursing into directories, and apply the permission fix in both the top-level fs::create_dir(state_path) path and the subdirectory fs::create_dir(&target) path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/migrations.rs` around lines 193 - 219, Update copy_dir_contents to
detect symlinks with entry.file_type().is_symlink() before directory recursion
and skip or safely copy them without following links, preventing recursive
self-copying. Also apply the required permission fix wherever migration creates
directories, including the top-level fs::create_dir(state_path) path and the
nested fs::create_dir(&target) path.
| // Ensure cache directories exist | ||
| if let Some(ref path) = cache_path { | ||
| std::fs::create_dir_all(path).ok(); | ||
| crate::core::paths::ensure_private_dir(path).ok(); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Propagate all ensure_private_dir failures.
Both call sites discard the helper's result. This bypasses the private-directory contract and hides persistence failures.
src/infra/player/streaming.rs#L759-L759: use?so credential-cache initialization stops when private-directory setup fails.src/infra/player/streaming.rs#L1291-L1291: propagate setup and write failures, and do not return an ID unless persistence succeeds.
📍 Affects 1 file
src/infra/player/streaming.rs#L759-L759(this comment)src/infra/player/streaming.rs#L1291-L1291
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/infra/player/streaming.rs` at line 759, Propagate all ensure_private_dir
failures instead of discarding them: update the credential-cache initialization
at src/infra/player/streaming.rs:759 to use ?, and update the persistence flow
at src/infra/player/streaming.rs:1291 to propagate both directory-setup and
write errors, returning an ID only after persistence succeeds.
Source: Coding guidelines
| .join("spotatui") | ||
| .join("streaming_cache") | ||
| }) | ||
| crate::core::paths::app_cache_dir().map(|dir| dir.join("streaming_cache")) |
There was a problem hiding this comment.
Blocking: this changes the Connect device identity for every existing user.
get_or_create_device_id reads device_id out of this directory, and its own comment says why: "so every launch and every in-app recovery registers as the same device (#297)". The directory moves config to cache with no migration, so after upgrade spotatui appears in Spotify as a brand new device. The CHANGELOG discloses a possible re-auth, not this.
migrate_legacy_path_if_unclaimed already handles directories, and its only directory test uses a streaming_cache fixture. One call here wires it up.
|
|
||
| let config_file_path = &app_config_dir.join(FILE_NAME); | ||
| let token_cache_path = &app_config_dir.join(TOKEN_CACHE_FILE); | ||
| let token_cache_path = &app_state_dir.join(TOKEN_CACHE_FILE); |
There was a problem hiding this comment.
Blocking, same shape as the streaming cache.
The token cache moves config to state with no rename, which is the re-auth the PR body flags. Since the shim now exists and handles files, one migrate_legacy_path_if_unclaimed call here turns the disclosed regression into a non-event.
| remove_behavior_keys_from_config(path, &keys) | ||
| } | ||
|
|
||
| fn remove_behavior_keys_from_config(path: &Path, keys: &[&str]) -> Result<bool> { |
There was a problem hiding this comment.
Blocking: this deletes a key the same PR documents.
docs/configuration.md gains an "Internet Radio" section telling users to preconfigure stations under behavior.radio_stations. On the first upgraded launch the shim copies that list into state.yml, state_owns_radio_stations returns true, and this function removes the key. A hand-written block is gone in the release that documents it.
The deletion exists only to stop the is_configured_radio_station_url guard from blocking removal of migrated favorites. Narrower fix: have the guard fire only when the URL is config-owned and not state-owned. Then nothing needs deleting, and the four runtime keys do not either, since persisted_state.X.is_some() already makes the shim idempotent.
That also drops the unprompted startup rewrite of config.yml, which matters given the PR's premise that the file stays declaratively manageable: write_private_file plus fs::rename replaces a home-manager symlink with a regular 0600 file.
| /// own one field, so blindly writing a whole in-memory snapshot would clobber | ||
| /// updates made by another running instance. | ||
| pub fn save(path: &Path, state: &PersistedRuntimeState) -> Result<()> { | ||
| let mut merged = load(path)?; |
There was a problem hiding this comment.
Non-blocking: a malformed state.yml never heals.
save starts with load(path)?, so once the file is corrupt every save fails forever. Startup only logs a warning, leaves state_path as None, and then each flush_state_save calls handle_error, which means a UI error on every volume nudge. A bad config.yml already falls back to defaults with a warning; suggest the same here, renaming to state.yml.bak and starting clean.
| if let Some(dir) = path.parent() { | ||
| crate::core::paths::ensure_private_dir(dir)?; | ||
| } | ||
| let tmp = path.with_extension("yml.tmp"); |
There was a problem hiding this comment.
Non-blocking, and only a partial close on the concurrency finding from last round.
Read-modify-write fixes clobber-by-omission, but two instances still lose updates: both load, both merge, both write. And the temp name is fixed, so two processes can write the same state.yml.tmp and rename each other's half-written file into place. A pid suffix is cheap. remove_behavior_keys_from_config has the same shape.
| crate::core::user_config::default_app_config_dir() | ||
| .map(|dir| dir.join(FILE_NAME)) | ||
| .ok_or_else(|| anyhow!("cannot resolve the spotatui config directory")) | ||
| crate::core::migrations::state_file_path_with_legacy_config_rename(Path::new(FILE_NAME)) |
There was a problem hiding this comment.
Non-blocking: this makes a path getter do filesystem work.
state_file_path_with_legacy_config_rename does create_dir_all, an unconditional chmod 0700, two stats, and a possible rename, on every call. That is once per throttled session save during playback and once per listen append in history.rs, forever, for a one-time upgrade. Running the shims once at startup would keep the getters pure and make the module easier to delete later, which is what its own doc comment asks for.
| ```bash | ||
| cp track-notifier.lua ~/.config/spotatui/plugins/ | ||
| case "${XDG_CONFIG_HOME:-}" in | ||
| /*) config_home="$XDG_CONFIG_HOME" ;; |
There was a problem hiding this comment.
Nit: this is more shell than the thing it installs.
The five-line case block is now in both README snippets and six plugin header comments, replacing a one-line cp. ${XDG_CONFIG_HOME:-$HOME/.config} covers the unset case inline; the relative-value case is rare enough to state once in PLUGINS.md. CodeRabbit asked for correctness here and got it, but the example headers are the first thing a plugin author reads.
|
Sorry this keeps growing, i know you've already done more rounds than you signed up for. If you'd rather hand any of it off, i'll push the fixes myself. |
|
Thanks man. I’m happy to take another pass at the changes, but if there’s another round after that, I’d be happy to hand the remaining fixes over to you. |
2 participants
Summary
Move generated runtime/app state out of the config directory and into XDG state/cache locations.
This separates user-authored configuration from app-generated state.
config.ymlremains suitable for hand editing or declarative management, while runtime changes such as volume, layout, active source, radio favorites, history, and token/cache files move to the appropriate XDG state/cache locations.$XDG_CONFIG_HOME/spotatui.state.yml, listening history, and Spotify OAuth token cache under$XDG_STATE_HOME/spotatui.$XDG_CACHE_HOME/spotatui/streaming_cache.Testing
cargo fmt --allcargo test --no-default-features --features telemetrycargo clippy --no-default-features --features telemetry -- -D warningscargo testcargo test --features all-sourcescargo clippy --features all-sources -- -D warningscargo checkAdditional notes
Existing Spotify OAuth token cache files under the old config directory are not migrated automatically. Users may need to authenticate again after this change.
Summary by CodeRabbit