fix(deezer): only disk-cache an album cover when the album lacks local art (#493) - #494
Conversation
…l art (#493) `enrich_album_inner` downloaded and cached the Deezer album cover into the shared `metadata_artwork` cache unconditionally. But it fires automatically on every album-page open — which reads only `label` + `release_date` — and from the Discord presence, which reads the remote `cover_url`; neither displays the downloaded file, and the album grid / detail header render the LOCAL artwork. So the cache steadily filled with never-shown Deezer covers for albums the user already had covers for (reported: a `metadata_artwork` folder full of album art the user never asked for). Gate the `download_and_cache` on `album.artwork_id IS NULL`: only a genuinely cover-less album gets its Deezer cover written to disk. The remote `cover_url` still rides through for Discord + the cache row, `label`/`release_date` are unchanged, and the deliberate `batch_fetch_missing_album_covers` (which iterates only `artwork_id IS NULL` albums) keeps working. No display regression — nothing rendered the downloaded file for an art-having album. Note: this stops new leaks; it does not prune covers already cached. A one-time cleanup (drop `metadata_album.cover_hash` files for albums with local art) can follow if wanted.
InstaZDLL
added
bug
|
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 (1)
📝 WalkthroughWalkthroughL’enrichissement Deezer ne télécharge plus de pochette lorsqu’un album possède déjà une illustration locale. Une commande Tauri supprime les références et les fichiers devenus inutiles. Les réglages affichent l’action, son état et son résultat dans toutes les langues prises en charge. ChangesGestion des pochettes Deezer
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
actor Utilisateur
participant SettingsView
participant Tauri
participant BasesProfils
participant CachePartage
Utilisateur->>SettingsView: lancer le nettoyage des pochettes
SettingsView->>Tauri: appeler prune_cached_album_covers
Tauri->>BasesProfils: effacer les références inutiles
Tauri->>CachePartage: supprimer les fichiers non référencés
CachePartage-->>Tauri: retourner les fichiers et octets supprimés
Tauri-->>SettingsView: retourner AlbumCoverCleanup
SettingsView-->>Utilisateur: afficher le résultat
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 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`:
- Around line 203-205: Corrigez le calcul de cover_hash autour de la branche
utilisant local_artwork_id et l’upsert associé afin qu’une entrée fraîche avec
artwork_id NULL et cover_hash NULL soit considérée incomplète, sans prolonger
son TTL. Lorsqu’aucun nouveau téléchargement n’est demandé, préservez le hash
déjà présent au lieu d’écrire NULL. Ajoutez des tests couvrant la suppression de
l’artwork local et le changement de profil, notamment via
batch_fetch_missing_album_covers.
- Around line 203-205: Ajoutez une vérification de `is_offline()` immédiatement
avant l’appel à `metadata_artwork::download_and_cache` dans le bras `(true,
Some(url))` de la correspondance `cover_hash`; évitez le téléchargement lorsque
le mode hors ligne est activé, tout en conservant le comportement actuel lorsque
le mode en ligne est actif.
🪄 Autofix
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: 0cb8d78a-c7a0-4bf9-91aa-07c25a6a37b4
📒 Files selected for processing (3)
CLAUDE.mddocs/features/integrations.mdsrc-tauri/crates/app/src/commands/deezer.rs
…ving albums (#493) Companion to the #493 fix, which stops NEW accumulation but leaves whatever the shared cache already collected. `prune_cached_album_covers` reclaims it: - Finds Deezer covers cached for albums that already have local artwork (`album.artwork_id IS NOT NULL`), clears their `metadata_album.cover_hash`, and deletes the `<hash>.jpg` + `_1x`/`_2x` files. - Reference-counted before any deletion: the cache is content-addressed and shared between artist pictures and album covers, so a hash is only unlinked once no `metadata_album.cover_hash` / `metadata_artist.picture_hash` / `background_hash` still points at it. Blocking fs work runs on spawn_blocking. - Scoped to the active profile; over-cleaning is self-healing since the #493 fix re-fetches a cover-less album's cover on next view. Surfaced as a "Prune unused Deezer album covers" card under Settings → Data (reports files deleted + MB freed). Frontend `pruneCachedAlbumCovers` wrapper + i18n `settings.pruneAlbumCovers*` ×17. Docs already covered by the #493 commit.
InstaZDLL
added
scope: frontend
|
Ajout du nettoyage ponctuel ( Nouvelle commande de maintenance
Validé : |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/maintenance.rs`:
- Around line 90-131: Encapsulate the candidate selection, cover_hash UPDATE,
and referenced-hash query in a single transaction within the cleanup function.
Begin a transaction from pool, execute each query through &mut *tx, and commit
only after all three operations succeed; preserve the existing empty-candidate
result and return behavior.
In `@src/components/views/SettingsView.tsx`:
- Around line 1166-1187: Update handlePruneCachedCovers to store a structured
success or failure result in pruneCoversStatus, setting the localized
settings.pruneAlbumCoversFailed message in the catch instead of leaving the
status null. Render failure distinctly from the green success styling, preserve
the existing reset behavior, and add the new localization key to all 17 locale
files.
In `@src/lib/tauri/library.ts`:
- Around line 233-235: Update pruneCachedAlbumCovers and its backend
implementation so album-cover selection, UPDATE, reference revalidation, and
file deletion are coordinated transactionally with Deezer scans and writers.
Revalidate references immediately before deletion, roll back database changes if
cleanup fails, and use mutable SQLite transactions with retry handling for
transient SQLITE_BUSY and SQLITE_LOCKED errors.
- Around line 233-235: Update the pruneCachedAlbumCovers flow and its backend
implementation so cleanup is scoped to the active profile and cannot null a
shared app.metadata_album reference used by another profile. Evaluate
eligibility across all profiles before clearing a shared deezer_id reference, or
make the reference profile-specific, while preserving the shared metadata cache
behavior.
🪄 Autofix
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: 016b7402-a3b5-422a-8898-cea8dd63e1b4
📒 Files selected for processing (21)
src-tauri/crates/app/src/commands/maintenance.rssrc-tauri/crates/app/src/lib.rssrc/components/views/SettingsView.tsxsrc/i18n/locales/ar.jsonsrc/i18n/locales/de.jsonsrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/hi.jsonsrc/i18n/locales/id.jsonsrc/i18n/locales/it.jsonsrc/i18n/locales/ja.jsonsrc/i18n/locales/ko.jsonsrc/i18n/locales/nl.jsonsrc/i18n/locales/pt-BR.jsonsrc/i18n/locales/pt.jsonsrc/i18n/locales/ru.jsonsrc/i18n/locales/tr.jsonsrc/i18n/locales/zh-CN.jsonsrc/i18n/locales/zh-TW.jsonsrc/lib/tauri/library.ts
…review) CodeRabbit round on #494. The download-skip introduced a cache-poisoning edge: `enrich_album_inner` returned any fresh `metadata_album` row as a hit, including one with `cover_hash = NULL`. So once an art-having album cached a cover-less row, a later need for its cover — the local art was removed, another profile shares the app-wide cache, or `batch_fetch_missing_album_covers` runs — was served the empty row and never re-fetched until the 30-day TTL lapsed. Gate the cache-hit on a completeness check: a fresh row is usable only when it carries a cover OR the album has its own local art (so it never needs the Deezer cover). An art-less, cover-less fresh row now falls through to Deezer. Extracted `metadata_album_cache_complete` + unit test. Skipped the paired "add an is_offline() guard before download_and_cache" suggestion: offline mode already returns at step 3, before the API call and the download, so that branch is unreachable offline — a second check would be dead code.
|
@coderabbitai Traité dans Finding 1 — une ligne fraîche sans cover bloque le re-fetch [FIXED] Fix = test de complétude dans le cache-hit : une ligne fraîche n'est un hit que si elle porte une cover OU si l'album a sa propre pochette locale (il n'aura jamais besoin de la cover Deezer). Une ligne fraîche sans cover pour un album sans pochette retombe donc vers Deezer. Extrait en Sur les deux sous-directives : plutôt que « préserver le hash au lieu d'écrire NULL » (ce qui garderait le fichier référencé et contredirait #493 — le but est justement de ne PAS conserver les covers des albums qui ont leur pochette, cf. la commande de purge) et « ne pas prolonger le TTL » (cas particulier fragile), le test de complétude surclasse le TTL exactement quand une cover est nécessaire → même intention (re-fetch possible), sans effet de bord. Finding 2 — Note tests : le prédicat est unit-testé ; un test d'intégration de |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
file="src-tauri/crates/app/src/commands/deezer.rs"
printf '%s\n' '== Target commit =='
git show --no-ext-diff --format=fuller --stat 94badcf7
printf '%s\n' '== File outline =='
ast-grep outline "$file" --items all --view expanded
printf '%s\n' '== Relevant symbols and call-site context =='
rg -n -C 12 \
'metadata_album_cache_complete|is_offline\(\)|download_and_cache|cover_hash|enrich_album_inner' \
"$file"
printf '%s\n' '== Target commit patch for the Deezer command =='
git show --no-ext-diff --format= -- "$file" 94badcf7Length of output: 27502
✏️ Learnings added
You are interacting with an AI system. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src-tauri/crates/app/src/commands/deezer.rs (2)
223-225: 🔒 Security & Privacy | 🟡 MinorVérifiez le mode hors ligne avant le téléchargement.
Le contrôle de la ligne 173 précède un
awaitvers Deezer. La ligne 224 ouvre un second chemin HTTP viametadata_artwork::download_and_cache. Si le mode hors ligne est activé pendant le premier appel, ce téléchargement peut encore partir. Ajoutez un contrôle juste avant l’appel, ou imposez ce contrôle dans le helper.#!/usr/bin/env bash set -euo pipefail rg -n -C 8 'download_and_cache(_full_res)?|is_offline' \ src-tauri/crates/core/src/artwork \ src-tauri/crates/app/src/commands/deezer.rsAs per coding guidelines, chaque chemin HTTP sortant doit vérifier
offline::is_offline()avant de continuer.🤖 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/deezer.rs` around lines 223 - 225, Add an offline-mode guard immediately before the `metadata_artwork::download_and_cache` call in the `cover_hash` match within the Deezer command, so the artwork HTTP request is skipped when `offline::is_offline()` is true. Preserve the existing matching behavior for local artwork and absent cover URLs, and follow the established offline check used earlier in the command.Source: Coding guidelines
223-225: 🗄️ Data Integrity & Integration | 🟠 MajorPréservez le
cover_hashexistant lorsque aucun nouveau fichier n’est écrit.Lorsque le cache a expiré et que
local_artwork_id.is_some(), cematchrenvoieNone. L’upsert des lignes 235-259 remplace alors un hash existant parNULL. Le fichier déjà stocké devient orphelin. La commande de maintenance ne peut plus retrouver ce hash pour le supprimer. Le même risque existe si le téléchargement best-effort échoue.Conservez l’ancien hash dans la branche sans téléchargement et lorsque le téléchargement renvoie
None. Ajoutez un test avec un cache expiré, une illustration locale et un hash existant.Correctif proposé
+ let mut cached_cover_hash: Option<String> = None; + if let Some(did) = existing_deezer_id { let cached: Option<( Option<String>, Option<String>, Option<String>, Option<String>, i64, )> = sqlx::query_as( "SELECT label, release_date, cover_url, cover_hash, expires_at FROM app.metadata_album WHERE deezer_id = ?", ) .bind(did) .fetch_optional(pool) .await?; + cached_cover_hash = cached + .as_ref() + .and_then(|(_, _, _, hash, _)| hash.clone()); @@ let cover_hash = match (local_artwork_id.is_none(), cover_url.as_deref()) { - (true, Some(url)) => metadata_artwork::download_and_cache(url, artwork_dir).await, - _ => None, + (true, Some(url)) => metadata_artwork::download_and_cache(url, artwork_dir) + .await + .or(cached_cover_hash), + _ => cached_cover_hash, };🤖 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/deezer.rs` around lines 223 - 225, Update the cover_hash handling in the artwork upsert flow to preserve the existing artwork hash whenever no download occurs or download_and_cache returns None, including expired-cache cases with local_artwork_id set. Only replace the hash after a successful new download, and add a test covering an expired cache, local artwork, and an existing hash.
🤖 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.
Duplicate comments:
In `@src-tauri/crates/app/src/commands/deezer.rs`:
- Around line 223-225: Add an offline-mode guard immediately before the
`metadata_artwork::download_and_cache` call in the `cover_hash` match within the
Deezer command, so the artwork HTTP request is skipped when
`offline::is_offline()` is true. Preserve the existing matching behavior for
local artwork and absent cover URLs, and follow the established offline check
used earlier in the command.
- Around line 223-225: Update the cover_hash handling in the artwork upsert flow
to preserve the existing artwork hash whenever no download occurs or
download_and_cache returns None, including expired-cache cases with
local_artwork_id set. Only replace the hash after a successful new download, and
add a test covering an expired cache, local artwork, and an existing hash.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e4f5bd4b-c0aa-4d68-b0bf-96a4daefb2c7
📒 Files selected for processing (1)
src-tauri/crates/app/src/commands/deezer.rs
…d download CodeRabbit round on #494. - deezer upsert: `cover_hash = COALESCE(excluded.cover_hash, cover_hash)` so a skipped (#493 art-having) or transiently-failed download no longer drops a good cached cover — only a NEW successful download overwrites the hash. - prune_cached_album_covers is now cross-profile safe: the metadata cache is app-wide, so a cover is prunable only when its album has local art in EVERY profile that carries it (read each profile's `album` table, build the "still needed" deezer_id set, exclude those). Prevents nulling a shared reference a different profile still relies on. The clear + reference re-check run in one transaction for a consistent snapshot. - Settings "Prune covers": the catch now shows a distinct red failure status (`settings.pruneAlbumCoversFailed`, ×17) instead of leaving it blank. Skipped, with reasons: - offline guard before download_and_cache: unreachable — the command already returns at the offline check (step 3) before the API call and the download. - SQLITE_BUSY/LOCKED retry loop: this is a foreground one-shot, not a background batch writer; a transient busy now surfaces via the failure status and the user retries — a retry loop would be disproportionate here. - integration test for the upsert / prune paths: the `waveflow` app-crate tests don't start locally (STATUS_ENTRYPOINT_NOT_FOUND, Tauri DLL) and both paths are DB+network bound with no injection point; CI runs them.
|
@coderabbitai Traité dans Cross-profil — nuller une référence partagée utilisée par un autre profil [FIXED] Transaction [FIXED] — le Préserver le hash quand pas de nouveau download [FIXED] — upsert : Statut d'échec UI [FIXED] — le Skips justifiés :
Validé : |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Commit summary and changed files =='
git show --no-ext-diff --format=fuller --stat 73b44822
git diff-tree --no-commit-id --name-only -r 73b44822
printf '%s\n' '== Relevant Rust symbols =='
for file in \
src-tauri/crates/app/src/commands/maintenance.rs \
src-tauri/crates/app/src/commands/deezer.rs \
src/components/views/SettingsView.tsx \
src/lib/tauri/library.ts
do
if [ -f "$file" ]; then
printf '\n== Outline: %s ==\n' "$file"
ast-grep outline "$file" --items all --view expanded || true
fi
done
printf '%s\n' '== Relevant implementation context =='
rg -n -C 14 \
'prune_cached_album_covers|cover_hash|picture_hash|background_hash|transaction|begin\(|COALESCE|metadata_album_cache_complete|pruneAlbumCovers|rowsCleared|filesDeleted|bytesFreed' \
src-tauri/crates/app/src/commands/maintenance.rs \
src-tauri/crates/app/src/commands/deezer.rs \
src/components/views/SettingsView.tsx \
src/lib/tauri/library.ts
printf '%s\n' '== Patch at target commit =='
git show --no-ext-diff --format= 73b44822 -- \
src-tauri/crates/app/src/commands/maintenance.rs \
src-tauri/crates/app/src/commands/deezer.rs \
src/components/views/SettingsView.tsx \
src/lib/tauri/library.ts \
src/i18n/localesLength of output: 110431
Les correctifs cross-profil, transaction, conservation de Il reste un problème de sûreté dans
La commande doit propager cette erreur au lieu de continuer avec une liste vide. ✏️ Learnings added
You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 2
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/maintenance.rs (1)
122-184: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSérialisez la suppression de fichiers avec les écritures du cache partagé.
Après le contrôle des références à la Line 164,
enrich_album_innerpeut restaurermetadata_album.cover_hashà la Line 251 pendant que cette commande supprime encore le fichier. La ligne de cache devient alors fraîche mais référence un fichier absent. Un album sans artwork local ne téléchargera pas de remplacement avant expiration du TTL.Utilisez un verrou partagé qui couvre les téléchargements, les upserts de
cover_hash, le contrôle des références et la suppression des fichiers.
src-tauri/crates/app/src/commands/maintenance.rs#L122-L184: prenez le verrou avant la sélection des candidats et conservez-le jusqu’à la fin dedelete_cover_files.src-tauri/crates/app/src/commands/deezer.rs#L237-L265: utilisez le même verrou pour toute opération qui télécharge ou écritmetadata_album.cover_hash.🤖 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/maintenance.rs` around lines 122 - 184, Serialize cache-file deletion with album-cover downloads and cache writes by introducing or reusing one shared lock. In src-tauri/crates/app/src/commands/maintenance.rs lines 122-184, acquire it before candidate selection and hold it through reference validation and delete_cover_files; in src-tauri/crates/app/src/commands/deezer.rs lines 237-265, use the same lock around the full operation that downloads artwork and writes metadata_album.cover_hash. Ensure both paths coordinate on the identical lock.
🤖 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/maintenance.rs`:
- Around line 87-90: Update prune_cached_album_covers to acquire the
profile-scoped pool with state.require_profile_pool().await? at the beginning
and retain the leased handle through the entire command. Use this handle for all
active-profile database access instead of obtaining or using an unleased pool.
- Around line 97-100: Dans le chargement de profile_ids de la commande de
maintenance, remplacez unwrap_or_default() par la propagation de l’erreur SQLx
afin qu’un échec de lecture de app_db interrompe la commande. Conservez le type
Vec<i64> et la requête existante, sans poursuivre avec une liste vide.
---
Outside diff comments:
In `@src-tauri/crates/app/src/commands/maintenance.rs`:
- Around line 122-184: Serialize cache-file deletion with album-cover downloads
and cache writes by introducing or reusing one shared lock. In
src-tauri/crates/app/src/commands/maintenance.rs lines 122-184, acquire it
before candidate selection and hold it through reference validation and
delete_cover_files; in src-tauri/crates/app/src/commands/deezer.rs lines
237-265, use the same lock around the full operation that downloads artwork and
writes metadata_album.cover_hash. Ensure both paths coordinate on the identical
lock.
🪄 Autofix
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: ca1ce2e6-f22b-456c-bd05-df9fa8643b08
📒 Files selected for processing (20)
src-tauri/crates/app/src/commands/deezer.rssrc-tauri/crates/app/src/commands/maintenance.rssrc/components/views/SettingsView.tsxsrc/i18n/locales/ar.jsonsrc/i18n/locales/de.jsonsrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/hi.jsonsrc/i18n/locales/id.jsonsrc/i18n/locales/it.jsonsrc/i18n/locales/ja.jsonsrc/i18n/locales/ko.jsonsrc/i18n/locales/nl.jsonsrc/i18n/locales/pt-BR.jsonsrc/i18n/locales/pt.jsonsrc/i18n/locales/ru.jsonsrc/i18n/locales/tr.jsonsrc/i18n/locales/zh-CN.jsonsrc/i18n/locales/zh-TW.json
CodeRabbit round on #494. `SELECT id FROM profile` used `unwrap_or_default()`: on a transient `app.db` read error the profile list fell back to empty, leaving the "still needed" set empty and marking EVERY cached cover prunable — a mass-deletion of covers other profiles need. Propagate the error instead.
|
@coderabbitai Traité dans
Verrou partagé download↔delete [SKIP — disproportionné] Validé : |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Commit and changed paths =='
git show --no-ext-diff --format=fuller --stat 1b12cc68
git diff-tree --no-commit-id --name-only -r 1b12cc68
printf '%s\n' '== Relevant outlines =='
ast-grep outline src-tauri/crates/app/src/commands/maintenance.rs --items all --view expanded
ast-grep outline src-tauri/crates/app/src/commands/deezer.rs --items all --view expanded
printf '%s\n' '== Maintenance implementation context =='
rg -n -C 18 \
'prune_cached_album_covers|SELECT id FROM profile|unwrap_or_default|profile_ids|profile_db|require_profile_pool|app_db|transaction|cover_hash|picture_hash|background_hash|spawn_blocking' \
src-tauri/crates/app/src/commands/maintenance.rs
printf '%s\n' '== Deezer write/download context =='
rg -n -C 14 \
'download_and_cache|cover_hash|metadata_album_cache_complete|if !.*exists|exists\(\)' \
src-tauri/crates/app/src/commands/deezer.rs
printf '%s\n' '== Patch at target commit =='
git show --no-ext-diff --format= 1b12cc68 -- \
src-tauri/crates/app/src/commands/maintenance.rs \
src-tauri/crates/app/src/commands/deezer.rsLength of output: 43384
Le finding sur Je ne relève pas de problème bloquant pour la fenêtre résiduelle entre le commit DB et la suppression de fichier. Le verrou global proposé aurait un coût disproportionné sur le chemin d’enrichissement. You are interacting with an AI system. |
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 (2)
src-tauri/crates/app/src/commands/maintenance.rs (2)
148-175: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftAjoutez des commits périodiques pour les gros nettoyages.
Si
prune_idscontient beaucoup d’entrées,chunks(400)limite seulement les paramètres SQL. La transaction reste ouverte jusqu’à la Line 175. Le verrou d’écriture SQLite peut bloquer les scans et les autres commandes pendant toute l’opération. Découpez le nettoyage en transactions bornées avec un état de reprise. Conservez le contrôle des références pour chaque lot.As per coding guidelines, « perform large writes in transactions with periodic commits ».
🤖 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/maintenance.rs` around lines 148 - 175, Découpez le nettoyage autour de la boucle chunks de la fonction concernée en transactions bornées, avec des commits périodiques afin de ne pas conserver le verrou d’écriture pendant toute l’opération. Ajoutez un état de reprise pour traiter chaque lot de prune_ids, puis recréez une transaction pour le lot suivant. Effectuez également le contrôle des références après chaque commit de lot, en conservant le nettoyage et le calcul rows_cleared.Source: Coding guidelines
166-186: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRendez le nettoyage du cache artwork récupérable et sérialisé.
prune_cached_album_coverscommittecover_hash = NULLavant la suppression des fichiers.delete_cover_filesignore les erreurs et aucun hash non supprimé n'est conservé pour une nouvelle tentative. Un fichier orphelin devient donc introuvable lors du nettoyage suivant. La transaction SQLite ne protège pas non plus la fenêtre avec les téléchargements et upserts demetadata_artistdansenrich_artist_deezer_inner.Ajoutez un verrou partagé avec les écritures du cache artwork et un état persistant de reprise. Ajoutez des tests pour les suppressions échouées, les interruptions et les références recréées pendant le nettoyage.
🤖 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/maintenance.rs` around lines 166 - 186, Rendez le flux prune_cached_album_covers récupérable et sérialisé avec les écritures du cache artwork : ajoutez un verrou partagé utilisé également par enrich_artist_deezer_inner et les upserts associés. Persistez les hashes dont les fichiers n’ont pas été supprimés afin de les retenter lors d’un nettoyage ultérieur, et ne perdez pas cet état en cas d’interruption ou d’échec de delete_cover_files. Ajoutez des tests couvrant les suppressions échouées, les interruptions et la recréation de références pendant le nettoyage.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.
Outside diff comments:
In `@src-tauri/crates/app/src/commands/maintenance.rs`:
- Around line 148-175: Découpez le nettoyage autour de la boucle chunks de la
fonction concernée en transactions bornées, avec des commits périodiques afin de
ne pas conserver le verrou d’écriture pendant toute l’opération. Ajoutez un état
de reprise pour traiter chaque lot de prune_ids, puis recréez une transaction
pour le lot suivant. Effectuez également le contrôle des références après chaque
commit de lot, en conservant le nettoyage et le calcul rows_cleared.
- Around line 166-186: Rendez le flux prune_cached_album_covers récupérable et
sérialisé avec les écritures du cache artwork : ajoutez un verrou partagé
utilisé également par enrich_artist_deezer_inner et les upserts associés.
Persistez les hashes dont les fichiers n’ont pas été supprimés afin de les
retenter lors d’un nettoyage ultérieur, et ne perdez pas cet état en cas
d’interruption ou d’échec de delete_cover_files. Ajoutez des tests couvrant les
suppressions échouées, les interruptions et la recréation de références pendant
le nettoyage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8230111e-104b-48c5-9722-b51166d6d491
📒 Files selected for processing (1)
src-tauri/crates/app/src/commands/maintenance.rs
|
@coderabbitai Vérifié contre le code actuel — les deux findings sont des SKIP motivés, pas de changement. Transactions bornées + commits périodiques + état de reprise par lot [SKIP] Verrou partagé + état de reprise persistant + tests [SKIP]
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
file="src-tauri/crates/app/src/commands/maintenance.rs"
printf '%s\n' '== Outline =='
ast-grep outline "$file" --items all --view expanded
printf '%s\n' '== Pruning transaction and filesystem context =='
rg -n -C 24 \
'prune_cached_album_covers|delete_cover_files|transaction|begin\(|commit\(|rollback\(|spawn_blocking|cover_hash|picture_hash|background_hash|UPDATE|SELECT' \
"$file"
printf '%s\n' '== Current command patch relative to PR base =='
base="$(git merge-base HEAD origin/main 2>/dev/null || true)"
if [ -n "$base" ]; then
git diff --no-ext-diff -- "$base" HEAD -- "$file"
else
git diff --no-ext-diff -- "$file"
fiLength of output: 24541
Des commits par lot empêcheraient le contrôle final de références de voir tous les Le risque résiduel entre le commit et la suppression de fichiers ne justifie pas un verrou global sur Je retire les deux findings. ✏️ Learnings added
You are interacting with an AI system. |
CodeRabbit round on #494. Format the freed megabytes with Intl.NumberFormat (i18n.resolvedLanguage ?? i18n.language) instead of toFixed(1), so locales that use a comma decimal separator render correctly (e.g. "12,5" in fr). Keeps the one-decimal precision; adds the language deps to the useCallback.
|
@coderabbitai Traité dans Formatage MB locale-aware [FIXED] Bail leasé Verrou partagé download↔delete tenu jusqu'après suppression [SKIP] Validé : |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Target commit =='
git show --no-ext-diff --format=fuller --stat e748c11d
git show --no-ext-diff --format= -- \
e748c11d -- \
src/components/views/SettingsView.tsx \
src-tauri/crates/app/src/commands/maintenance.rs
printf '%s\n' '== Settings view outline and formatter context =='
ast-grep outline src/components/views/SettingsView.tsx --items all --view expanded
rg -n -C 12 \
'Intl\.NumberFormat|toFixed\(|pruneCachedAlbumCovers|resolvedLanguage|i18n\.language|useCallback' \
src/components/views/SettingsView.tsx
printf '%s\n' '== Maintenance outline and transaction context =='
ast-grep outline src-tauri/crates/app/src/commands/maintenance.rs --items all --view expanded
rg -n -C 18 \
'prune_cached_album_covers|require_profile_pool|profile_db|state\.app_db|transaction|begin\(|commit\(|cover_hash|picture_hash|background_hash|spawn_blocking|delete_cover_files' \
src-tauri/crates/app/src/commands/maintenance.rsLength of output: 57467
Le formatage est corrigé.
Le ✏️ Learnings added
You are interacting with an AI system. |
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 (2)
src/components/views/SettingsView.tsx (2)
1194-1196: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winÉvitez qu’un ancien minuteur efface le résultat suivant.
Si l’utilisateur relance le nettoyage avant l’expiration des cinq secondes, le
setTimeoutde la première exécution reste actif. Il peut appelersetPruneCoversStatus(null)pendant ou après la deuxième exécution. Le résultat le plus récent peut donc disparaître trop tôt.Conservez l’identifiant du minuteur dans un
useRef, annulez le minuteur précédent avant d’en créer un nouveau et nettoyez-le au démontage.Correction proposée
+ const pruneCoversStatusTimerRef = useRef<number | null>(null); + + useEffect(() => { + return () => { + if (pruneCoversStatusTimerRef.current !== null) { + window.clearTimeout(pruneCoversStatusTimerRef.current); + } + }; + }, []); + const handlePruneCachedCovers = useCallback(async () => { ... } finally { setIsPruningCovers(false); - window.setTimeout(() => setPruneCoversStatus(null), 5000); + if (pruneCoversStatusTimerRef.current !== null) { + window.clearTimeout(pruneCoversStatusTimerRef.current); + } + pruneCoversStatusTimerRef.current = window.setTimeout(() => { + setPruneCoversStatus(null); + pruneCoversStatusTimerRef.current = null; + }, 5000); }Le risque vient du minuteur local créé dans ce gestionnaire.
🤖 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/components/views/SettingsView.tsx` around lines 1194 - 1196, Update the pruning cleanup flow around setPruneCoversStatus to store the timeout identifier in a useRef, clear any existing timeout before scheduling a new one, and assign the new identifier to the ref. Add unmount cleanup to cancel the active timeout and prevent stale timers from clearing a newer pruning result.
3682-3691: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAnnoncez le résultat aux technologies d’assistance.
Le résultat est inséré après une commande asynchrone, mais le
<div>n’a niroleniaria-live. Un lecteur d’écran peut ne pas annoncer le nombre de fichiers supprimés, l’espace libéré ou l’échec.Correction proposée
<div + role={pruneCoversStatus.ok ? "status" : "alert"} + aria-live={ + pruneCoversStatus.ok ? "polite" : "assertive" + } + aria-atomic="true" className={`text-xs mt-1 truncate ${As per path instructions, cette vérification couvre les « problèmes d’accessibilité visibles » du frontend.
🤖 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/components/views/SettingsView.tsx` around lines 3682 - 3691, Ajoutez une annonce destinée aux technologies d’assistance sur le conteneur rendu par la condition pruneCoversStatus, en lui attribuant un aria-live approprié et, si nécessaire, un rôle de statut. Conservez le texte et les styles existants afin que les succès comme les échecs annoncent le résultat asynchrone.Source: Path instructions
🤖 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/components/views/SettingsView.tsx`:
- Around line 1194-1196: Update the pruning cleanup flow around
setPruneCoversStatus to store the timeout identifier in a useRef, clear any
existing timeout before scheduling a new one, and assign the new identifier to
the ref. Add unmount cleanup to cancel the active timeout and prevent stale
timers from clearing a newer pruning result.
- Around line 3682-3691: Ajoutez une annonce destinée aux technologies
d’assistance sur le conteneur rendu par la condition pruneCoversStatus, en lui
attribuant un aria-live approprié et, si nécessaire, un rôle de statut.
Conservez le texte et les styles existants afin que les succès comme les échecs
annoncent le résultat asynchrone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1d93afbd-5c35-40b1-ad78-66f373734ef3
📒 Files selected for processing (1)
src/components/views/SettingsView.tsx
CodeRabbit round on #494. Two small SettingsView fixes: - Keep the auto-clear timeout in a ref, cancelling any prior one before scheduling a new one and on unmount, so a rapid re-run (or unmount) can't have a stale timer wipe a newer prune result / fire after teardown. - Give the prune status container role="status" + aria-live="polite" so both the success and failure outcomes are announced to assistive tech.
|
@coderabbitai Traité dans Timer de statut (fuite / timer périmé) [FIXED] — le Annonce a11y [FIXED] — le conteneur de statut porte maintenant Validé : |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
file="src/components/views/SettingsView.tsx"
printf '%s\n' '== Commit =='
git show --no-ext-diff --format=fuller --stat 848add82
git show --no-ext-diff --format= 848add82 -- "$file"
printf '%s\n' '== Component outline =='
ast-grep outline "$file" --items all --view expanded
printf '%s\n' '== Timer and status context =='
rg -n -C 14 \
'useRef|useEffect|setTimeout|clearTimeout|handlePruneCachedCovers|pruneStatus|role="status"|aria-live' \
"$file"Length of output: 46567
Le timeout de statut est stocké dans Le conteneur de statut a You are interacting with an AI system. |
1 participant
Fixes #493.
The bug
AppData\...\waveflow\metadata_artworkfills with Deezer album covers the user never asked for — even with the "fetch album covers" batch never run, and even for albums that already have local artwork.Root cause
enrich_album_innerdownloads + caches the Deezer cover unconditionally, but every automatic caller ignores that file:AlbumDetailView.tsx) firesenrichAlbumDeezeron every open and reads onlylabel+release_date— never the cover.discord_presence.rs) reads the remotecover_url, not the downloaded file.aw.hash);get_album_detailjoinsmetadata_albumonly forlabel.So the downloaded cover is pure dead weight for any album that has its own art.
Fix
Gate the
download_and_cacheonalbum.artwork_id IS NULL— only a genuinely cover-less album writes a Deezer cover to disk.cover_urlstill rides through for Discord + the cache row;label/release_dateunchanged; the deliberatebatch_fetch_missing_album_covers(alreadyWHERE artwork_id IS NULL) is unaffected. No display regression — nothing showed that file for an art-having album.Scope note
This stops new accumulation. It does not prune covers already cached. A one-time cleanup (delete
metadata_artworkfiles referenced bymetadata_album.cover_hashfor albums whose localartwork_id IS NOT NULL, and null thosecover_hash) can be a follow-up if wanted — kept out here to keep the fix focused and low-risk.Validation
cargo clippy -p waveflow --all-targetsclean.metadata_artworkno longer grows; Discord album art (remote URL) and label/release-date still work; a cover-less album still gets its fallback + the Settings "fetch missing covers" batch still fills them.Docs:
docs/features/integrations.md+ CLAUDE.md catalogue updated.Summary by CodeRabbit
Nouvelles fonctionnalités
Améliorations
Documentation