feat(similar-artists): backfill Deezer pictures when Last.fm wins#156
Conversation
… cascade
Last.fm's artist.getSimilar has been returning the same generic star
placeholder image URL for every result since they retired their artist-
image endpoint in 2019. With a Last.fm key configured, the similar-
artists carousel was therefore showing a sea of grey stars for every
entry that wasn't already in the user's library — only in-library hits
got a picture, courtesy of the existing profile-DB Deezer cache. Users
without a Last.fm key got a better visual outcome because the cascade
fell through to Deezer's /artist/{id}/related, which returns real photo
URLs.
Fix the asymmetry by routing the raw cascade output through a new
enrich_with_deezer_pictures helper before responding. It joins against
the cross-profile app.metadata_artist cache by canonical name, fans out
parallel Deezer search_artist calls for cache misses (≤ 12 in flight),
persists the new rows + downloads the image into the shared
metadata_artwork cache, and merges everything into a per-name
HashMap<canonical_name, (picture_url, picture_hash)> the final DTO
mapping consumes. Result: same UX whether Last.fm is configured or not,
plus a permanent disk cache so subsequent loads are network-free.
No-op when offline mode is on (stale metadata cache only) — matches the
existing offline behaviour of every other Deezer-enrichment path.
InstaZDLL
added
scope: backend
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAjout d'un enrichissement d'images Deezer pour les artistes similaires Last.fm : la fonction ChangesEnrichissement d'images Deezer pour similar artists
Sequence Diagram(s)sequenceDiagram
participant get_similar_artists
participant enrich_with_deezer_pictures
participant metadata_artist as app.metadata_artist
participant DeezerAPI as Deezer search_artist
participant artwork as artwork download
get_similar_artists->>enrich_with_deezer_pictures: canonical names
enrich_with_deezer_pictures->>metadata_artist: charge entrées non expirées
enrich_with_deezer_pictures->>DeezerAPI: search_artist parallèle (misses)
DeezerAPI->>enrich_with_deezer_pictures: picture_url + artist data
enrich_with_deezer_pictures->>artwork: télécharge artwork si disponible
artwork->>enrich_with_deezer_pictures: retourne picture_hash local
enrich_with_deezer_pictures->>metadata_artist: upsert avec expires_at (TTL 30j)
enrich_with_deezer_pictures-->>get_similar_artists: map canonical→(url, hash)
get_similar_artists->>get_similar_artists: fusionne dans DTO retourné
Estimated code review effort🎯 3 (Modéré) | ⏱️ ~25 minutes 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: 3
🤖 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/similar.rs`:
- Around line 243-247: La lecture cache et l’upsert avalent les erreurs
actuellement — remplace l’if let Ok(rows) = q.fetch_all(pool).await { ... }
silencieux par une gestion d’erreur explicite: utilisez match ou l’opérateur ?
sur q.fetch_all(pool).await dans la fonction contenant canonical_name et
map.insert pour propager une Err(Result) ou logger l’erreur (par ex. via
tracing::error!) avec le contexte ("fetch_all for cache" ou "upsert cache")
avant de retourner/propager l’erreur; applique le même changement aux blocs
similaires autour des symboles q.fetch_all, map.insert et toute fonction
d’upsert dans les sections correspondantes (lines ~301-319).
- Around line 252-287: The current fan-out launches a request per item in
misses, ignoring the "≤ 12" concurrency cap and the later take(RESULT_LIMIT);
change the pipeline so you only process up to RESULT_LIMIT entries from misses
and run at most 12 concurrent Deezer requests: replace the join_all over
misses.iter() with a stream-based approach (e.g.,
futures::stream::iter(misses.into_iter().take(RESULT_LIMIT)).map(|r| async move
{ ... }).buffer_unordered(CONCURRENCY_LIMIT).collect()) using a constant
CONCURRENCY_LIMIT = 12 and the existing RESULT_LIMIT, keeping the same logic
inside the async block that calls DeezerClient::search_artist and canonical_name
to produce (canon, hit).
- Around line 224-241: The SQL comparison uses LOWER(TRIM(name)) but the bound
values in canonicals were produced by canonical_name(), so names with
punctuation (e.g., AC/DC, P!nk) mismatch; update the WHERE clause so the DB-side
expression matches canonical_name() normalization (for example apply the same
lowercase+strip-non-alnum transformation on name using a regexp replace) and
keep binding canonicals as-is; change the SQL string built in sql (used to
create q) to use that canonicalized expression against the ({placeholders}) list
so app.metadata_artist lookups align with canonical_name().
🪄 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: 90592c25-3848-4e4c-9841-7fc1bc3262f5
📒 Files selected for processing (3)
CLAUDE.mddocs/features/integrations.mdsrc-tauri/src/commands/similar.rs
Three issues raised in PR #156 review, all valid against the current code: 1. **Silent failure swallowing.** `if let Ok(rows) = q.fetch_all(...)` and `let _ = sqlx::query(...).execute(...)` were dropping DB errors on the floor — direct violation of the project's no-silent-failures policy. Both now match the Err arm and log via `tracing::warn!` with the right context (cache lookup vs upsert). 2. **Concurrency cap absent + over-enrichment.** `join_all` over the raw miss list could fan out > RESULT_LIMIT requests when the Deezer fallback returned > 12 hits, and the surplus was thrown away by `.take(RESULT_LIMIT)` downstream. Switched to `futures::stream::iter(...).buffer_unordered(CONCURRENCY_LIMIT)` and sliced misses to `raw[..RESULT_LIMIT]` upfront so every fired request maps to an entry that'll actually display. 3. **Canonical-name SQL mismatch.** The cache lookup used `LOWER(TRIM(name))` while `canonicals` came from `canonical_name()` which strips non-alphanumerics. Artists like AC/DC and P!nk would never match, forcing a Deezer round-trip every request. SQLite's stock build has no REGEXP function so an equivalent SQL predicate isn't reachable without a stored column + migration. Switched to a `WHERE expires_at > ?` scan + Rust-side `canonical_name()` filter — table is bounded at ~thousands of rows so the scan is sub-ms, and the comparison is now byte-for-byte identical to the scanner's. Side effects: 1) offline mode now serves stale cache instead of returning an empty map (the cache read is local); 2) `DeezerClient` derives `Clone` so the buffered stream can stamp it into each future without HRTB lifetime grief — internal field is already Arc-backed so clone is a refcount bump.
|
@coderabbitai Addressed all three findings in 49bc763. 1. Silent failure swallowing — fixed 2. Concurrency cap + RESULT_LIMIT trimming — fixed
3. Canonical-name SQL mismatch — fixed (chose Rust-side filter) SQLite's standard build has no REGEXP function and no user-defined alphanumeric filter, so an equivalent SQL predicate isn't reachable without a stored canonical column + migration. Took the simpler path: dropped the Bonus side effect: offline mode now serves stale cache instead of returning an empty map (the cache read is local — no reason to skip it). Matches the existing offline behaviour of CI: |
|
Tip For best results, initiate chat on the files or code changes.
Nothing blocking. 🐰 |
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/similar.rs`:
- Around line 249-253: La requête actuelle utilise sqlx::query_as::<_, (String,
Option<String>, Option<String>)> avec "FROM app.metadata_artist WHERE expires_at
> ?" et la branche offline renvoie immédiatement le map, ce qui fait perdre les
images locales quand le TTL est expiré; changez la logique pour appeler
offline::is_offline() en amont et, si offline, exécuter une requête sans le
filtre "WHERE expires_at > ?" sur app.metadata_artist pour lire le cache local
(y compris picture_url/picture_hash) puis court-circuiter toute tentative de
refresh réseau; appliquez la même modification aux autres occurrences
mentionnées (les blocs autour des lignes équivalentes 291-294).
🪄 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: 63615fe8-f418-4dba-b1ca-edc08415d015
📒 Files selected for processing (3)
docs/features/integrations.mdsrc-tauri/src/commands/similar.rssrc-tauri/src/deezer.rs
The cache lookup applied `WHERE expires_at > ?` unconditionally and the offline check sat downstream. Net effect: when the user was offline AND the metadata_artist row had expired, the enrichment helper returned an empty map for that artist — even though the `metadata_artwork/<hash>.jpg` blob was still on disk and a stale picture is strictly better than a grey star (we have no way to refresh the metadata until network returns anyway). Split the cache query in two: the offline branch reads `SELECT … FROM app.metadata_artist` (no TTL filter) and short-circuits before the miss computation; the online branch keeps the `WHERE expires_at > ?` predicate so expired rows correctly fall through to a Deezer refresh. The shared metadata_artwork blob cache has no TTL, so any hash we return that still has a file on disk will render — `existing_path` already guards the resolution. Hashes whose file was purged just degrade to `picture_url` (remote) which won't load offline but matches the pre-change behaviour for missing files.
|
@coderabbitai Addressed in b70c1c4. Finding valid. Confirmed against current code: the Fix: split the cache query in two paths based on
The shared Both occurrences the reviewer pointed at (cache lookup ~249-253 + offline short-circuit ~291-294) handled in the same refactor. The miss-computation block now only runs when Tests: 111 passed. |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✓ |
1 participant
Summary
Last.fm's
artist.getSimilarreturns the same generic star placeholder URL for every entry — their artist-image endpoint was retired in 2019. With a Last.fm key configured, the "Similar artists" carousel onArtistDetailViewwas showing a sea of grey stars for every result that wasn't already in the user's library. Users without Last.fm got a better outcome because the cascade fell through to Deezer's/artist/{id}/related, which returns real photos.Fix the asymmetry by routing the raw cascade output through a new
enrich_with_deezer_pictureshelper before responding.How it works
app.metadata_artistcache byLOWER(TRIM(name))— pulls every entry we already have a Deezer picture for, no matter which profile fetched it originally.search_artistfor cache misses viafutures::future::join_all(≤ 12 in flight, bounded byRESULT_LIMIT).metadata_artwork::download_and_cache(url, dir)writes the JPEG once (blake3-keyed, dedup across artists),INSERT … ON CONFLICT(deezer_id) DO UPDATErefreshes the metadata row + the 30-day TTL.picture_url, and resolvespicture_pathfrom the in-library hash first →metadata_artist.picture_hashfallback →None.Caching
app.metadata_artist) — 30-day TTL, shared between profiles.metadata_artwork/<blake3>.jpg) — disk cache, no TTL, dedups identical pictures across artists.offline::is_offline(), serving stale cache only).What didn't change
app.lastfm_similarcache (still stores rawRawSimilarrows with the Last.fm placeholder URL — enrichment lives at the read path, not the write path, so the picture stays fresh independently of the affinity scores).resolveRemoteImage(picture_path, picture_url)resolution — the priority logic already prefers local paths.Test plan
cargo test --manifest-path src-tauri/Cargo.toml --lib→ 111 passed.cargo check --all-targetsclean.Summary by CodeRabbit
Nouvelles fonctionnalités
Documentation