fix(similar): drop Deezer's empty-hash placeholder pictures (#406)#431
Merged
Conversation
Deezer serves a grey-silhouette placeholder when an artist has no real photo: the CDN path carries an empty image hash (`/images/artist//…`, double slash) or the md5 of the empty string. We were accepting those as if they were real pictures — the "similar artist shows no photo" half of #406, and worse, caching the grey blob into metadata_artist so it stuck. Add `DeezerArtistHit::best_picture()` (largest real size first, skipping the placeholder) plus a shared `is_placeholder_artist_picture` in waveflow-core, and route every point that accepts a Deezer artist picture through it: the similar-artist enrichment + related-artist fallback (similar.rs) and the artist enrichment / search / single-artist fetch (deezer.rs). Since every size shares the one hash, a placeholder in xl means all sizes are placeholders → None, and the artist correctly falls back to its initial-letter avatar instead of a grey box. The 30-day cache-poisoning on a *network* failure was already fixed in #412 (failures no longer cached); this closes the remaining "real answer, but the picture is a placeholder" gap.
InstaZDLL
added
scope: backend
|
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:
📝 WalkthroughWalkthroughLa sélection d’images Deezer filtre les placeholders et choisit la meilleure taille disponible. Cette logique est appliquée aux flux d’enrichissement, de recherche, d’artwork et d’artistes similaires, avec normalisation des données de cache Deezer et Last.fm. ChangesImages d’artistes Deezer
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DeezerClient
participant DeezerCommands
participant MetadataCache
participant SimilarArtists
participant SimilarArtistDto
DeezerClient->>DeezerCommands: fournit DeezerArtistHit
DeezerCommands->>DeezerCommands: sélectionne best_picture()
MetadataCache->>SimilarArtists: fournit les métadonnées en cache
SimilarArtists->>SimilarArtists: neutralise les placeholders Deezer et Last.fm
SimilarArtists->>SimilarArtistDto: compose les artistes similaires
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 2
🤖 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/crates/app/src/commands/deezer.rs`:
- Line 493: Les anciennes valeurs du cache Deezer peuvent encore contenir des
placeholders et contourner le filtrage appliqué aux réponses fraîches. Dans
enrich_artist_deezer_inner (src-tauri/crates/app/src/commands/deezer.rs, lignes
493-493), validez ou invalidez picture_url et picture_hash avant de retourner
l’enrichissement ; appliquez la même validation dans enrich_with_deezer_pictures
(src-tauri/crates/app/src/commands/similar.rs, lignes 437-437) avant d’alimenter
la carte des résultats.
In `@src-tauri/crates/core/src/metadata/deezer.rs`:
- Around line 257-266: Corriger le test
best_picture_skips_a_placeholder_and_takes_the_next_real_size pour fournir une
URL xl placeholder suivie d’une URL de taille inférieure réelle, puis vérifier
que best_picture() retourne cette image réelle au lieu de None.
🪄 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: a9650487-708e-4c2b-967b-b71a45790472
📒 Files selected for processing (3)
src-tauri/crates/app/src/commands/deezer.rssrc-tauri/crates/app/src/commands/similar.rssrc-tauri/crates/core/src/metadata/deezer.rs
The placeholder filter only ran on fresh Deezer responses, so a row written before this change still held a placeholder URL (and a grey-blob picture_hash) and bypassed it on the cache-hit read paths. Filter on read too: - enrich_artist_deezer_inner: when the cached row is fresh enough to return without a refetch, drop a placeholder picture_url + hash so the artist detail falls back to the initial-letter avatar. A placeholder means the artist has no real Deezer photo, so there is nothing to heal to; the row's own TTL refresh re-fetches through best_picture anyway. - enrich_with_deezer_pictures: drop a placeholder from a cached metadata_artist row before it enters the similar-artist result map. Also fix the test whose name promised the skip behaviour but asserted None: it now feeds a placeholder xl + a real lower size and checks the real one is returned, with a separate test for the all-placeholder case.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/crates/app/src/commands/similar.rs`:
- Around line 369-375: Update the metadata handling around metadata_map so an
existing normalized entry of (None, None) remains authoritative and does not
fall back to r.picture_url. Ensure Last.fm placeholder URLs are discarded after
normalization, while preserving fallback behavior only when no metadata entry
exists.
- Around line 369-375: Preserve the absence of a valid Deezer image in both
similar-artist paths: update the cache-entry handling around the existing
`entry` tuple at lines 369-375 and the `best_picture()` handling at lines
444-448 so `(None, None)` remains absent instead of falling back to
`r.picture_url` or the Last.fm placeholder. Apply the same no-fallback behavior
in both sites.
🪄 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: e2683d1c-42bb-4dc0-965a-69bfbf577ef5
📒 Files selected for processing (3)
src-tauri/crates/app/src/commands/deezer.rssrc-tauri/crates/app/src/commands/similar.rssrc-tauri/crates/core/src/metadata/deezer.rs
…m's star Nulling a placeholder in the metadata_artist cache was undone downstream: the DTO build did `meta.url.or(r.picture_url)`, so a `(None, None)` entry — an artist we enriched and confirmed has no real Deezer photo — fell back to `r.picture_url`, which for a Last.fm-sourced similar entry is the generic star placeholder. The grey/star box came back. Make a present metadata entry authoritative: its URL wins, `None` included, and only a never-enriched artist (`meta` is `None`) still falls back to the upstream URL as a best-effort remote. Apply the same guard to the curated-similar path (#323), which read `ma.picture_url` straight from the JOIN: a pre-#406 placeholder there is now dropped (URL + cached hash) while the local sidecar picture, resolved separately, is untouched.
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/crates/app/src/commands/similar.rs`:
- Around line 202-212: Update the local_map query and mapping logic near the
existing picture_url selection to also read ma.picture_url, then apply
is_placeholder_artist_picture() so placeholder metadata clears the returned
picture hash to None. Preserve local_map.picture_hash for non-placeholder images
and keep the metadata_map authority limited to picture_url.
🪄 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: dd54fffb-1ffd-4b50-acf8-32b3fa7a3f2d
📒 Files selected for processing (1)
src-tauri/crates/app/src/commands/similar.rs
…ture path The local_map query reads ma.picture_hash straight from metadata_artist without the placeholder filter, so a library artist whose grey-silhouette Deezer picture was cached before the #406 fix still resolves picture_path to that blob on disk. The commit-2 read-time filter only covered the cross-profile meta path; this closes the same gap for library entries by reading ma.picture_url alongside and clearing the hash when it points at a placeholder. picture_url authority stays with the metadata_map.
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/crates/app/src/commands/similar.rs (1)
212-222: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNe réintroduisez pas le placeholder Last.fm lorsque
metaest absent.Lorsque l’enrichissement Deezer est désactivé ou indisponible,
meta == Noneet cette branche renvoier.picture_url. Pour les résultats Last.fm, cette valeur est l’URL générique placeholder ; sanspicture_pathlocal, l’UI affiche donc encore l’image grise au lieu de l’avatar à initiale prévu par le PR. Filtrez explicitement ce placeholder dans ce cas, ou retournezNonepour les résultats Last.fm non enrichis.🤖 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/crates/app/src/commands/similar.rs` around lines 212 - 222, Update the picture_url selection around the meta match so unenriched Last.fm results do not return the generic placeholder URL. When meta is None, filter that known placeholder or return None, while preserving authoritative metadata_artist URLs and allowing genuine upstream fallback URLs.
🤖 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/crates/app/src/commands/similar.rs`:
- Around line 212-222: Update the picture_url selection around the meta match so
unenriched Last.fm results do not return the generic placeholder URL. When meta
is None, filter that known placeholder or return None, while preserving
authoritative metadata_artist URLs and allowing genuine upstream fallback URLs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 16c94ebb-8ae6-4c98-a1eb-3a70c019bffc
📒 Files selected for processing (1)
src-tauri/crates/app/src/commands/similar.rs
…iched When Deezer enrichment yields no entry (meta is None) — offline mode, or no Deezer hit for that name — the picture_url fell back to r.picture_url, which for a Last.fm getSimilar result is the generic md5-named star placeholder. Add is_lastfm_star_placeholder() in core (co-located with pick_largest_image, the star's source) and filter it in the None branch so the UI shows its initial-letter avatar instead. metadata_artist URLs stay authoritative; a genuine upstream picture still passes through.
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/crates/app/src/commands/similar.rs`:
- Around line 224-227: Update the None fallback in the picture_url match within
the similar-results flow to reject both Last.fm and Deezer placeholder URLs
before returning the cached r.picture_url. Add a regression test covering an
offline/cache payload with a Deezer placeholder and verify that it is filtered
out.
🪄 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: 0ab8e123-5c2f-4aec-bf6e-b3f502745d5f
📒 Files selected for processing (2)
src-tauri/crates/app/src/commands/similar.rssrc-tauri/crates/core/src/metadata/lastfm.rs
…lback A similar payload cached in app.lastfm_similar before the #406 fix still holds Deezer's empty-hash silhouette URL (best_picture() only filters fresh fetches). On a cache-hit the None branch surfaced it because the fallback filter only knew Last.fm's star. Extract unenriched_fallback_ picture() rejecting both sentinels, with a regression test covering the offline/cache Deezer-placeholder case.
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 the remaining "similar artist shows no photo" half of #406.
Context
#406 was closed after #412 fixed the main symptom (a network failure was being cached empty for 30 days, poisoning the artist's similar list). jo-el414 traced the deeper cause to Deezer's own API changes. The fuzzy name matcher and don't-cache-network-failures levers are already in.
What was left: a successful Deezer answer can still carry a placeholder image. Deezer serves a grey silhouette when an artist has no real photo — the CDN path has an empty image hash (
/images/artist//500x500…, double slash) or the md5 of the empty string. We accepted those as real pictures and cached the grey blob intometadata_artist, so it stuck across sessions.Fix
waveflow-core:is_placeholder_artist_picture(url)+DeezerArtistHit::best_picture()— largest real size first, skipping the placeholder. Every size shares the one hash, so a placeholder inpicture_xlmeans all sizes are placeholders →None, and the UI falls back to the initial-letter avatar instead of a grey box.similar.rs), and artist enrichment / search / single-artist fetch (deezer.rs). Filtering at every write into the shared cache is what stops a placeholder from entering it via any path.Reference
The placeholder pattern was confirmed against how rustmusic handles the same Deezer quirk (
is_deezer_default_image). The caching model itself (retry-on-empty vs sticky) was cross-checked against lokal's per-field TTL design — no change needed there, #412 already covers the failure case, and with Deezer's API degraded, aggressively retrying genuine empties would just hammer a broken endpoint.Testing
5 unit tests in
waveflow-core(empty-hash + md5 detection, real-hash acceptance, best_picture skip/prefer/none) — run locally, all pass.cargo clippy -p waveflow --all-targetsclean.Summary by CodeRabbit