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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughSpotatui separates startup configuration from persisted runtime state. It adds XDG-aware config, cache, and state paths, migrates legacy data, and updates playback, layout, radio, UI, plugin, and documentation code. 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.
| 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.
|
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
| /// 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.
|
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. |
Resolved CHANGELOG.md conflict from upstream merge.
|
I've made another round of changes. I'll leave #407 (comment) and #407 (comment) to you, since I'm not confident I can make a set of changes there that would be up to the project's standard. Also, I found it funny that you read my intent here as making the project compatible with Home Manager configuration. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/infra/player/streaming.rs (1)
1524-1574: 🩺 Stability & Availability | 🔵 TrivialMigration now preserves the Connect device identity; consider surfacing a failed migration to the user.
get_default_cache_pathnow callsmigrate_legacy_streaming_cache_if_unclaimedbefore returning the new cache path, andlegacy_streaming_cache_migration_preserves_connect_device_idconfirmsdevice_idandcredentials.jsonsurvive the move. This resolves the earlier concern that upgrading would silently register spotatui as a brand-new Spotify Connect device.One gap remains: when migration fails, the code only logs a warning and continues with the new, empty cache path. In that case the device still re-registers as new, the same user-visible symptom as before, just now triggered by a filesystem error instead of "always". Consider surfacing this failure through a status message at startup (once the runtime plumbing for it exists) instead of relying on a log line most users never see.
🤖 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` around lines 1524 - 1574, Update get_default_cache_path to surface migration failures through the existing startup status-message mechanism once runtime plumbing is available, rather than only emitting log::warn. Preserve returning the new cache path, and include enough context in the user-visible message to indicate that legacy streaming cache migration failed.
🤖 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/accent-cycler.lua`:
- Line 6: Update the plugin installation commands to resolve XDG_CONFIG_HOME
only when it is absolute, otherwise falling back to $HOME/.config before
appending the Spotatui plugins directory. Apply this consistently in
examples/plugins/accent-cycler.lua lines 6-6,
examples/plugins/now-playing-webhook.lua lines 7-7,
examples/plugins/now-playing.lua lines 6-8,
examples/plugins/track-info-popup.lua lines 4-6, and
examples/plugins/track-notifier.lua lines 4-4.
---
Nitpick comments:
In `@src/infra/player/streaming.rs`:
- Around line 1524-1574: Update get_default_cache_path to surface migration
failures through the existing startup status-message mechanism once runtime
plumbing is available, rather than only emitting log::warn. Preserve returning
the new cache path, and include enough context in the user-visible message to
indicate that legacy streaming cache migration failed.
🪄 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: fa2856cb-9ff8-433d-8782-befa86572e93
📒 Files selected for processing (20)
CHANGELOG.mdPLUGINS.mddocs/keybindings.mdexamples/plugins/README.mdexamples/plugins/accent-cycler.luaexamples/plugins/now-playing-webhook.luaexamples/plugins/now-playing.luaexamples/plugins/queue-browser.luaexamples/plugins/session-stats/main.luaexamples/plugins/track-info-popup.luaexamples/plugins/track-notifier.luasrc/core/app.rssrc/core/config.rssrc/core/migrations.rssrc/core/persisted_playback.rssrc/infra/history.rssrc/infra/player/streaming.rssrc/runtime.rssrc/tui/handlers/playlist.rssrc/tui/runner.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- examples/plugins/session-stats/main.lua
- src/infra/history.rs
- CHANGELOG.md
- PLUGINS.md
- src/tui/runner.rs
- examples/plugins/README.md
- docs/keybindings.md
- examples/plugins/queue-browser.lua
- src/core/app.rs
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 config-dir app data is migrated on first use when the new target path does not already exist. This includes legacy runtime fields/radio favorites from
config.yml, listening history,last_session.yml, Spotify OAuth token caches, and native streaming credentials/audio cache.If a new state/cache target already exists, the legacy file or directory is left in place instead of being merged or overwritten; users may need to move or remove legacy files manually only in that conflict case.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation