fix: No lyrics for this track appear even when LRCLIB has the specific song - #411
Merged
Merged
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughLyrics retrieval now uses a shared LRCLIB helper. It tries an exact lookup, then fuzzy search when needed. The code validates lyric content, selects results by lyric quality and duration, and updates lyrics state consistently. ChangesLyrics retrieval
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant get_lyrics
participant fetch_lrclib_lyrics
participant LRCLIB
participant LyricsState
get_lyrics->>fetch_lrclib_lyrics: request lyrics
fetch_lrclib_lyrics->>LRCLIB: try exact lookup with rounded duration
LRCLIB-->>fetch_lrclib_lyrics: return exact response
fetch_lrclib_lyrics->>LRCLIB: search when the exact response has no lyrics
LRCLIB-->>fetch_lrclib_lyrics: return search results
fetch_lrclib_lyrics-->>get_lyrics: return selected lyrics
get_lyrics->>LyricsState: store lyrics and update generation
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.
🧹 Nitpick comments (1)
src/infra/network/utils.rs (1)
536-561: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for unknown duration and missing result duration.
The three tests cover the main ranking path. Two branches in
pick_search_resultstay untested:duration <= 0.0at Line 390, and a result whosedurationisNoneat Lines 391-394. Both branches decide which result a user sees when metadata is incomplete.🧪 Proposed additional tests
#[test] fn search_returns_none_when_no_result_has_lyrics() { let results = vec![search_result(None, Some(" "), Some(200.0))]; assert!(pick_search_result(results, 200.0).is_none()); } + + #[test] + fn search_ignores_duration_when_track_duration_unknown() { + let results = vec![ + search_result(Some("[00:01.00] first"), None, Some(900.0)), + search_result(Some("[00:01.00] second"), None, Some(10.0)), + ]; + let picked = pick_search_result(results, 0.0).unwrap(); + assert_eq!(picked.syncedLyrics.as_deref(), Some("[00:01.00] first")); + } + + #[test] + fn search_deprioritizes_result_without_duration() { + let results = vec![ + search_result(Some("[00:01.00] no duration"), None, None), + search_result(Some("[00:01.00] has duration"), None, Some(300.0)), + ]; + let picked = pick_search_result(results, 200.0).unwrap(); + assert_eq!(picked.syncedLyrics.as_deref(), Some("[00:01.00] has duration")); + }Run these with
cargo test --no-default-features --features telemetry.As per coding guidelines: "Run tests with
cargo test --no-default-features --features telemetryand add or adjust tests when behavior changes."🤖 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/network/utils.rs` around lines 536 - 561, Add tests near the existing pick_search_result tests covering an unknown target duration (duration <= 0.0) and a candidate whose duration is None. Assert each case selects the expected lyric result according to pick_search_result’s fallback behavior, using the existing search_result helper and test command.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 `@src/infra/network/utils.rs`:
- Around line 536-561: Add tests near the existing pick_search_result tests
covering an unknown target duration (duration <= 0.0) and a candidate whose
duration is None. Assert each case selects the expected lyric result according
to pick_search_result’s fallback behavior, using the existing search_result
helper and test command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 149a698b-0a45-4a5d-a484-db2f96454011
📒 Files selected for processing (1)
src/infra/network/utils.rs
LargeModGames
added a commit
that referenced
this pull request
Aug 5, 2026
# Summary Fixes #410. Lyrics never appeared for tracks credited to more than one artist (e.g. "Take Me Back" by Kygo and Max McNown), even when LRCLIB clearly had the track. Root cause: LRCLIB indexes each track under a single artist string (almost always the primary credit), but spotatui sent the full **joined** credit (`"Kygo, Max McNown"`) to both the exact `/api/get` and the fuzzy `/api/search` endpoints, so every collaboration missed. The `PlaybackMetadata.artists` field was typed `Vec<String>` but in practice always held one pre-joined display string, so there was no way to recover the individual artists downstream. The fix threads the real per-artist list through instead of a display string: - The playback snapshot now carries individual artist names as a structured list (both the native-streaming and Spotify-context paths). `NativeTrackInfo.artists_display: String` became `artists: Vec<String>`, populated from the structured list already present at the player-event boundary. - The LRCLIB lookup (`network/utils.rs`) now tries a candidate ladder: all `/api/get` candidates first, then all `/api/search`, where candidates are `[full joined credit, primary artist alone]`. The primary artist comes from the structured list (never a `", "` split), so an act whose own name contains a comma (e.g. "Earth, Wind & Fire") plus a guest is not corrupted. Side effect: MPRIS `xesam:artist` is now a genuine array instead of a single joined element. Display is unchanged (`primary_artist()` still joins the list), and the friends-widget backend already joins the artists array with `", "`, so there is no wire-format change. # Testing - `cargo fmt --all` - `cargo clippy --no-default-features --features telemetry -- -D warnings` (clean) - `cargo clippy -- -D warnings` (default, clean) - `cargo clippy --features all-sources -- -D warnings` (clean) - `cargo test --no-default-features --features telemetry` (510 passed) - `cargo test` (default, 771 passed) - `cargo test --features all-sources` (843 passed) - Added unit tests for the artist candidate ladder and a snapshot regression test for a multi-artist native track. - Live: played "Take Me Back" (Kygo, Max McNown) on the native device and the synced lyrics now render. # Additional notes The earlier fix for this issue (#411) added the `/api/get` -> `/api/search` fallback but still passed the joined multi-artist string to both endpoints, so collaborations stayed broken; this addresses the underlying data shape. Local-files (single lofty artist string) and internet radio (no track duration, so no lyrics) are intentionally left as-is. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Lyrics now load correctly for collaboration tracks. - Lookup tries the complete artist credits first, then falls back to the primary artist. - Artist names containing commas are handled correctly. - Artist display formatting remains unchanged in the player interface. - **Tests** - Added coverage for solo artists, collaborations, fallback lookups, and empty artist lists. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #410.
Summary
No lyrics for this track appear even when LRCLIB has the specific song
Validation
🤖 AI-authored PR, operated by @yhay81.
Summary by CodeRabbit
New Features
Bug Fixes
Tests