various: add offline scrobbling support#511
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a DB-backed offline scrobble queue, wires enqueue/drain paths into scheduler and startup logic, and updates scrobble constructors to accept explicit timestamps. It also simplifies macOS callback storage and replaces transmute-based pointer casts. ChangesOffline Scrobble Queue
macOS System Integration Cleanup
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 2
🧹 Nitpick comments (4)
crates/hooks/src/scrobble_scheduler.rs (1)
276-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting
Credentials-from-config into a shared helper.This
Credentialsconstruction is duplicated inuse_player_controller.rs(Lines 1029-1046), and the two copies disagree on Libre.fm gating: here it is gated onhas_librefm(which includesoptions.include_librefm), whereas the startup path gates only on a non-emptylibrefm_session_key. A single helper (e.g.Credentials::from_config(&AppConfig)) would remove the drift and keep drain behavior consistent across both call sites.The
drop(cfg)beforedrain(...).awaitcorrectly avoids holding a signal borrow across the await.🤖 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 `@crates/hooks/src/scrobble_scheduler.rs` around lines 276 - 293, Extract the scrobble queue Credentials construction into a shared helper, since scrobble_scheduler::drain and the startup path in use_player_controller both build the same scrobble::queue::Credentials but with different Libre.fm gating. Add a single source of truth such as Credentials::from_config or a small helper near scrobble::queue, and update both call sites to use it so lastfm, librefm, and listenbrainz fields are derived consistently from AppConfig and the current feature flags.Source: Coding guidelines
crates/scrobble/src/queue.rs (3)
101-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for
is_transient.This function is the sole arbiter of whether a failed scrobble is queued for retry or permanently dropped. Misclassification in either direction is a data-loss bug (dropping retryable scrobbles) or a resource leak (infinitely re-queueing permanent failures). The existing test suite covers persistence and merge logic but not this classification.
🧪 Suggested tests
#[test] fn is_transient_no_status_is_transient() { // A connection/timeout error has no HTTP status → transient. // Constructing a reqwest::Error without status requires a builder // or a specific error kind; use reqwest::Error::builder if available, // or test indirectly via a mock that returns a connect error. // At minimum, document the expected behavior for: // - None (no response) → transient // - 500 (server error) → transient // - 400 (client error) → not transient // - 401 (auth failure) → not transient }🤖 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 `@crates/scrobble/src/queue.rs` around lines 101 - 106, Add unit tests for the is_transient function in scrobble::queue to cover the retry classification behavior. Create focused tests around is_transient that verify no HTTP status is treated as transient, server errors like 500 are transient, and client/auth errors like 400 and 401 are not transient. Use the is_transient symbol directly so the tests stay aligned with the retry/drop decision logic.
172-236: 🧹 Nitpick | 🔵 TrivialAt-least-once delivery: crash between submit and checkpoint causes duplicate scrobbles.
drainsubmits to the service (line 190) and checkpoints to disk afterward (lines 223-233). If the process exits in that window, the next drain re-submits the same item. Most scrobbling services deduplicate by timestamp+artist+track, so this is likely harmless in practice — but worth documenting as a known property of the design.🤖 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 `@crates/scrobble/src/queue.rs` around lines 172 - 236, Document the at-least-once behavior in drain so it’s clear that submit_one can succeed before the checkpoint write in ScrobbleQueue::drain, and a crash in that window may cause the same scrobble to be resent on the next run. Add a concise note near the drain flow or its public docs mentioning the submit-then-checkpoint design and that duplicate delivery is an expected tradeoff, since most services deduplicate by timestamp/artist/title.
58-76: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBlocking file I/O in async context.
ScrobbleQueue::loadandsaveusestd::fs::read_to_string/write/rename— blocking calls invoked from asyncenqueueanddrain. For a small local config file this is negligible in practice, but it can stall the tokio worker thread on slow filesystems. Considertokio::fsorspawn_blockingif this path ever moves to a network-backed config dir.🤖 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 `@crates/scrobble/src/queue.rs` around lines 58 - 76, ScrobbleQueue::load and ScrobbleQueue::save perform blocking std::fs I/O that can stall the tokio runtime when called from async enqueue and drain. Update the queue persistence path to use non-blocking filesystem APIs such as tokio::fs, or wrap the existing file operations in spawn_blocking if you want to keep the current implementation. Keep the changes localized to the ScrobbleQueue load/save methods and the async callers that invoke them.
🤖 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 `@crates/hooks/src/use_player_controller.rs`:
- Around line 1023-1048: The startup scrobble drain in
use_player_controller::use_future is running before App has finished hydrating
config, so it can build empty scrobble::queue::Credentials and clear the offline
queue via scrobble::queue::drain. Gate this effect on the loaded-config state,
or add a dedicated “scrobble creds ready” check before constructing the
credentials and calling drain, so the startup retry only runs once the real
config values are available.
In `@crates/scrobble/src/queue.rs`:
- Around line 277-278: Queued ListenBrainz submits are dropping
scheduler-provided metadata because submit_one() always calls
musicbrainz::make_listen with None for additional_info. Update QueuedScrobble to
store the attached payload (duration and MBIDs) alongside the item, then have
submit_one() pass that saved value through when rebuilding the listen. Make sure
the queue/drain path preserves and reuses the same metadata field that the live
submission path attaches.
---
Nitpick comments:
In `@crates/hooks/src/scrobble_scheduler.rs`:
- Around line 276-293: Extract the scrobble queue Credentials construction into
a shared helper, since scrobble_scheduler::drain and the startup path in
use_player_controller both build the same scrobble::queue::Credentials but with
different Libre.fm gating. Add a single source of truth such as
Credentials::from_config or a small helper near scrobble::queue, and update both
call sites to use it so lastfm, librefm, and listenbrainz fields are derived
consistently from AppConfig and the current feature flags.
In `@crates/scrobble/src/queue.rs`:
- Around line 101-106: Add unit tests for the is_transient function in
scrobble::queue to cover the retry classification behavior. Create focused tests
around is_transient that verify no HTTP status is treated as transient, server
errors like 500 are transient, and client/auth errors like 400 and 401 are not
transient. Use the is_transient symbol directly so the tests stay aligned with
the retry/drop decision logic.
- Around line 172-236: Document the at-least-once behavior in drain so it’s
clear that submit_one can succeed before the checkpoint write in
ScrobbleQueue::drain, and a crash in that window may cause the same scrobble to
be resent on the next run. Add a concise note near the drain flow or its public
docs mentioning the submit-then-checkpoint design and that duplicate delivery is
an expected tradeoff, since most services deduplicate by timestamp/artist/title.
- Around line 58-76: ScrobbleQueue::load and ScrobbleQueue::save perform
blocking std::fs I/O that can stall the tokio runtime when called from async
enqueue and drain. Update the queue persistence path to use non-blocking
filesystem APIs such as tokio::fs, or wrap the existing file operations in
spawn_blocking if you want to keep the current implementation. Keep the changes
localized to the ScrobbleQueue load/save methods and the async callers that
invoke them.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: eecc94d8-7ade-4a07-9818-a8169000fea8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/hooks/src/scrobble_scheduler.rscrates/hooks/src/use_player_controller.rscrates/scrobble/Cargo.tomlcrates/scrobble/src/lastfm.rscrates/scrobble/src/lib.rscrates/scrobble/src/librefm.rscrates/scrobble/src/queue.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/player/src/systemint/macos.rs (1)
191-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCasts are sound, but update the now-stale SAFETY comment above.
Swapping
transmutefor reference-to-pointerascasts is correct here. However, the SAFETY block at Lines 159-164 still explains soundness in terms of "transmute from &NSString to *const c_void", which no longer matches the code. Refresh the wording to describe theascasts / toll-free bridging so the rationale stays accurate.As per coding guidelines: "Keep comments focused on the non-obvious reason why something exists; do not restate the code."
🤖 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 `@crates/player/src/systemint/macos.rs` around lines 191 - 193, The SAFETY comment in the macOS assertion setup is stale and still describes a transmute-based conversion, while the code now uses reference-to-pointer as casts for toll-free bridging. Update the comment near the assertion creation logic in the macos system interface to match the current cast approach and explain the soundness in terms of objc2_foundation::NSString being bridged to const c_void. Keep it focused on the non-obvious safety rationale and remove wording that restates the code.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@crates/player/src/systemint/macos.rs`:
- Around line 191-193: The SAFETY comment in the macOS assertion setup is stale
and still describes a transmute-based conversion, while the code now uses
reference-to-pointer as casts for toll-free bridging. Update the comment near
the assertion creation logic in the macos system interface to match the current
cast approach and explain the soundness in terms of objc2_foundation::NSString
being bridged to const c_void. Keep it focused on the non-obvious safety
rationale and remove wording that restates the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b911b9e8-10cd-4807-ab99-0869fbe16fd1
📒 Files selected for processing (1)
crates/player/src/systemint/macos.rs
UMCEKO
left a comment
UMCEKO
left a comment
There was a problem hiding this comment.
We should not use json for this
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/hooks/src/use_player_controller.rs (1)
1027-1056: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDelay the startup drain until config is populated
config_loaded_okflips beforeconfig.set(...), so this effect can run once againstAppConfig::default().drain()treats missing creds asNoCredentialsand checkpoints those services out of the queue, which permanently drops queued scrobbles instead of retrying them. Move the flag set to afterconfig.set(...)or make the effect depend on the actual creds-ready state.🤖 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 `@crates/hooks/src/use_player_controller.rs` around lines 1027 - 1056, The startup scrobble drain is running before the real config is available, so use_effect in use_player_controller can build Credentials from AppConfig::default() and trigger scrobble::queue::drain with missing auth. Move the config_loaded_ok update until after config.set(...) completes, or gate this effect on a creds-ready state derived from config, so drain only runs once valid Last.fm/Libre.fm/ListenBrainz credentials are populated.
🤖 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.
Outside diff comments:
In `@crates/hooks/src/use_player_controller.rs`:
- Around line 1027-1056: The startup scrobble drain is running before the real
config is available, so use_effect in use_player_controller can build
Credentials from AppConfig::default() and trigger scrobble::queue::drain with
missing auth. Move the config_loaded_ok update until after config.set(...)
completes, or gate this effect on a creds-ready state derived from config, so
drain only runs once valid Last.fm/Libre.fm/ListenBrainz credentials are
populated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fd99fea6-8134-44c9-a15f-88cc9b738b2d
📒 Files selected for processing (4)
crates/hooks/src/scrobble_scheduler.rscrates/hooks/src/use_player_controller.rscrates/kopuz/src/main.rscrates/scrobble/src/queue.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/scrobble/src/queue.rs
- crates/hooks/src/scrobble_scheduler.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/hooks/src/scrobble_scheduler.rs (1)
279-293: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBuild drain credentials from raw Libre.fm credential presence.
has_librefmis gated byoptions.include_librefm, so aScrobbleOptions::LOCALscrobble can passNoneforlibrefm_session_keyintoscrobble::queue::drain(). That makes queued Libre.fm rows look delivered viaOutcome::NoCredentials, and they get dropped.🤖 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 `@crates/hooks/src/scrobble_scheduler.rs` around lines 279 - 293, Build the scrobble drain Credentials from the actual presence of the Libre.fm session key instead of the include_librefm flag, so scrobble::queue::drain receives a usable librefm_session_key whenever one exists. In scrobble_scheduler::scrobble_ok, update the creds construction around Credentials and the has_librefm logic so LOCAL scrobbles with a populated Libre.fm key are not treated as NoCredentials and dropped from the queue.
🤖 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 `@crates/scrobble/src/queue.rs`:
- Around line 102-127: The Outcome::NoCredentials arm in the match statement on
submit_one() currently sets delivered to true, which silently and permanently
deletes queued scrobbles without distinguishing between temporary credential
absence and permanent removal. To preserve data integrity when credentials are
transiently unavailable, modify the Outcome::NoCredentials case to behave like
Outcome::Transient by adding the service to give_up and setting delivered to
false instead, ensuring rows remain in the queue until credentials return.
Additionally, update the test drain_without_credentials_clears_queue to reflect
this changed behavior.
---
Outside diff comments:
In `@crates/hooks/src/scrobble_scheduler.rs`:
- Around line 279-293: Build the scrobble drain Credentials from the actual
presence of the Libre.fm session key instead of the include_librefm flag, so
scrobble::queue::drain receives a usable librefm_session_key whenever one
exists. In scrobble_scheduler::scrobble_ok, update the creds construction around
Credentials and the has_librefm logic so LOCAL scrobbles with a populated
Libre.fm key are not treated as NoCredentials and dropped from the queue.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f7ed8fe-49b1-4762-9104-5f3370b5f76b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
crates/db/migrations/20260709000000_scrobble_queue.sqlcrates/db/src/backend/mod.rscrates/db/src/backend/scrobble_queue.rscrates/db/src/lib.rscrates/hooks/src/scrobble_scheduler.rscrates/hooks/src/use_player_controller.rscrates/scrobble/Cargo.tomlcrates/scrobble/src/lib.rscrates/scrobble/src/queue.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/scrobble/Cargo.toml
2 participants
i took the stuff from pr #445 and yeah thats it but i will improve this fixed #335
Sanity Checking
rules.
contribution guidelines, or this pull request did not use AI assistance.
Style and Consistency
style.
cargo fmt --all --checkorcargo fmt --allas appropriate.cargo clippy --workspace --all-targets -- -D warnings, orexplained why it could not be run.
this change depends on them.
Testing
Tested on platform(s):
x86_64-linuxaarch64-linuxx86_64-darwinaarch64-darwin