feat(lyrics): auto-discover local sidecar .lrc / .txt files (#115)#117
Merged
Conversation
Many users keep their lyrics as sidecar files next to the audio
(\`01 Song.mp3\` + \`01 Song.lrc\` / \`.txt\`) or in a sibling
\`Lyrics/\` subfolder — common K-Pop / J-Pop rip layout. Until now
those files were invisible to WaveFlow unless the user pressed
"Import File" per track, which doesn't scale to large libraries.
Slot a new tier between the embedded-tag read and the LRCLIB
network call in the \`fetch_lyrics\` waterfall:
1. DB cache
2. Embedded tag (USLT / TXXX / Vorbis / MP4 ©lyr)
3. NEW: Sidecar file
4. LRCLIB
The sidecar lookup probes:
- {audio_dir}/{stem}.{lrc|txt} (same folder, primary)
- {audio_dir}/Lyrics/{stem}.{lrc|txt} (sibling subfolder)
Both stem and extension matching are case-insensitive so a
mixed-case rip like \`Song.MP3\` finds \`song.lrc\` cleanly on
case-sensitive filesystems. \`.lrc\` wins over \`.txt\` at every
probed directory because it carries timing info, and same-folder
hits beat \`Lyrics/\` hits. Whitespace-only files fall through
instead of caching an empty entry.
The library-wide \`prefetch_library_lyrics\` walks the same new
waterfall, so the "Prefetch lyrics" button now also surfaces
local sidecars without network traffic.
\`save_lyrics\` still writes only to the embedded tag, so the
sidecar file stays read-only: the file's authoritative copy
becomes the embedded one after any in-app edit (the new file
hash invalidates the cache row, the next fetch sees the embedded
tag, sidecar is silently superseded).
9 new unit tests cover the helper's matching rules
(same-folder, subfolder case-insensitive, stem case-insensitive,
.lrc-wins-over-.txt, same-folder-beats-subfolder, empty file,
missing file, etc). All 17 \`commands::lyrics\` tests pass.
Docs updated: CLAUDE.md playback catalogue and
docs/features/integrations.md LRCLIB waterfall section.
InstaZDLL
added
scope: backend
📝 WalkthroughWalkthroughCe PR ajoute la lecture de fichiers lyrics locaux ( ChangesSupport des fichiers lyrics sidecar locaux
Sequence Diagram(s)sequenceDiagram
participant Client
participant fetch_lyrics
participant DBCache
participant EmbeddedTags
participant SidecarReader
participant LRCLib
Client->>fetch_lyrics: request lyrics(track)
fetch_lyrics->>DBCache: lookup by track.file_hash
alt cache hit
DBCache-->>fetch_lyrics: cached payload
fetch_lyrics-->>Client: return cached payload
else cache miss
fetch_lyrics->>EmbeddedTags: read embedded tag (USLT/TXXX)
alt embedded hit
EmbeddedTags-->>fetch_lyrics: embedded payload
fetch_lyrics-->>DBCache: upsert(source=Embedded)
fetch_lyrics-->>Client: return embedded payload
else embedded miss
fetch_lyrics->>SidecarReader: spawn_blocking read_sidecar_lyrics(path)
alt sidecar hit
SidecarReader-->>fetch_lyrics: sidecar payload / format
fetch_lyrics-->>DBCache: upsert(source=LrcFile)
fetch_lyrics-->>Client: return sidecar payload
else sidecar miss
fetch_lyrics->>LRCLib: network lookup
LRCLib-->>fetch_lyrics: remote payload
fetch_lyrics-->>DBCache: upsert(source=LRCLib)
fetch_lyrics-->>Client: return remote payload
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🤖 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 `@src-tauri/src/commands/lyrics.rs`:
- Around line 380-399: The loop over entries currently treats directory names
like "Song.lrc" as candidates and later fails when trying to read them; update
the loop that sets lrc_match and txt_match (the for entry in entries.flatten()
block that uses path, file_stem, lrc_match, txt_match and produces chosen) to
skip non-regular files before evaluating file_stem and extension — e.g., check
that the entry/path is a file (via entry.file_type()/metadata() or
path.is_file()) and continue if not, so only real files are considered for
.lrc/.txt selection.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 65c75f2c-2549-4689-a771-bd2681b808f1
📒 Files selected for processing (3)
CLAUDE.mddocs/features/integrations.mdsrc-tauri/src/commands/lyrics.rs
CodeRabbit caught a real edge case: a directory named `Song.lrc` next to a track would be selected into `lrc_match`, `read_to_string` on it would fail, and a legitimate `Song.txt` fallback in the same directory would be silently masked. Skip non-file entries up front with `path.is_file()` so only real files are considered. `is_file` follows symlinks so a symlinked sidecar (one master `lyrics/` folder linked from album directories) still works. Added a regression test that reproduces the original failure (directory `Song.lrc` shadowing valid `Song.txt`) and asserts the fallback now wins.
Owner
Author
|
Applied in 78d9af6. Real edge case caught — a directory named Fix: Added a regression test |
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)
src-tauri/src/commands/lyrics.rs (1)
382-413:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNe laisse pas un
.lrcvide masquer un.txtvalide.Le
is_file()corrige bien le cas du dossier, mais la sélection reste faite avant lecture. Du coup, siSong.lrcexiste mais ne contient que des espaces, la fonction renvoieNoneau lieu de retomber surSong.txt, alors que les sidecars vides sont censés être ignorés.Patch proposé
- let chosen = lrc_match.or(txt_match)?; - let raw = std::fs::read_to_string(&chosen).ok()?; - let trimmed = raw.trim(); - if trimmed.is_empty() { - None - } else { - Some(trimmed.to_string()) - } + for candidate in [lrc_match, txt_match].into_iter().flatten() { + let raw = match std::fs::read_to_string(&candidate) { + Ok(raw) => raw, + Err(_) => continue, + }; + let trimmed = raw.trim(); + if !trimmed.is_empty() { + return Some(trimmed.to_string()); + } + } + NoneJe rajouterais aussi un test de régression pour le couple
.lrcvide +.txtvalide.🤖 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-tauri/src/commands/lyrics.rs` around lines 382 - 413, The current selection picks lrc_match over txt_match before reading, so an .lrc file that is empty (only whitespace) will mask a valid .txt; change the logic to prefer a non-empty sidecar: try reading lrc_match first and if reading succeeds but trimmed.is_empty() then fall back to reading txt_match (or vice‑versa if you prefer txt priority), i.e., after computing lrc_match and txt_match, attempt read_to_string(&lrc_match) -> trimmed and only return it if non-empty, otherwise attempt read_to_string(&txt_match) and return that if non-empty; update the code paths around chosen/raw/trimmed to perform this fallback and add a regression test that creates an empty Song.lrc and a valid Song.txt to assert the .txt is used.
🤖 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 `@src-tauri/src/commands/lyrics.rs`:
- Around line 382-413: The current selection picks lrc_match over txt_match
before reading, so an .lrc file that is empty (only whitespace) will mask a
valid .txt; change the logic to prefer a non-empty sidecar: try reading
lrc_match first and if reading succeeds but trimmed.is_empty() then fall back to
reading txt_match (or vice‑versa if you prefer txt priority), i.e., after
computing lrc_match and txt_match, attempt read_to_string(&lrc_match) -> trimmed
and only return it if non-empty, otherwise attempt read_to_string(&txt_match)
and return that if non-empty; update the code paths around chosen/raw/trimmed to
perform this fallback and add a regression test that creates an empty Song.lrc
and a valid Song.txt to assert the .txt is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 02b96f91-d8bd-4386-a144-0c381b748181
📒 Files selected for processing (1)
src-tauri/src/commands/lyrics.rs
Real edge case from the review: a `Song.lrc` that exists but is empty / whitespace-only would short-circuit the waterfall. `lrc_match.or(txt_match)` always picked the lrc, the trimmed read returned None, and a perfectly valid `Song.txt` sibling was silently lost (we fell through to LRCLIB instead). Try each candidate individually: read the lrc, if its trimmed content is non-empty use it, otherwise read the txt. Extracted the read+trim+is_empty dance into a `read_non_empty_file` helper so both candidates share the same logic. Added regression test `sidecar_empty_lrc_falls_back_to_txt` that reproduces the original failure (empty `.lrc` + valid `.txt`) and asserts the `.txt` content wins. All 19 `commands::lyrics` tests pass.
Owner
Author
|
Applied in 49f8d8a. Real bug — an empty Fix: try each candidate individually, fall back to New regression test |
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #115.
Summary
Many users keep their lyrics as sidecar files next to the audio (`01 Song.mp3` + `01 Song.lrc` / `.txt`) or in a sibling `Lyrics/` subfolder — a very common K-Pop / J-Pop rip layout. Until now those files were invisible to WaveFlow unless the user pressed "Import File" per track, which doesn't scale beyond a handful of songs.
Slot a new tier between the embedded-tag read and the LRCLIB network call in the `fetch_lyrics` waterfall.
New waterfall
Sidecar lookup rules
The library-wide `prefetch_library_lyrics` walks the same new waterfall, so the "Prefetch lyrics" button now also surfaces local sidecars without network traffic.
Why `save_lyrics` was left alone
`save_lyrics` keeps writing only to the embedded tag. The sidecar file stays read-only on purpose: the file's authoritative copy becomes the embedded one after any in-app edit (the new file hash invalidates the cache row, the next fetch sees the embedded tag, the sidecar is silently superseded). This avoids two confusing failure modes:
Test plan
Backend
Frontend
Manual
Docs
Summary by CodeRabbit
Notes de version
Nouvelles fonctionnalités
Comportement
Documentation
Tests