feat(artist): split a comma-joined phantom artist in place (#396)#433
Open
InstaZDLL wants to merge 1 commit into
Open
feat(artist): split a comma-joined phantom artist in place (#396)#433InstaZDLL wants to merge 1 commit into
InstaZDLL wants to merge 1 commit into
Conversation
A comma-joined ARTIST tag ("A, B, C") is linked to a single phantom
artist because the scanner splits on "; " only (a comma can be part of a
real name — "Tyler, The Creator"). The track then dead-ends on the
phantom while the real, enriched artists already exist next to it.
Add a user-driven escape hatch (option B from the issue): the phantom's
Edit-info modal shows a "Split" section (only when the name contains a
comma) previewing the comma fragments; confirming calls the new
split_artist command, which re-links every crediting track to the
individual artists — reusing existing rows by canonical name so the
track immediately points at the already-enriched artist — repoints
track.primary_artist, and deletes the phantom once nothing references it
(guarded on track_artist / album.artist_id / track.primary_artist /
artist_similar_custom). No file is re-tagged.
Durability: the scanner skip-branch now treats a comma-joined tag (one
"; "-split name) whose track already credits several artists as a
deliberate split and leaves it alone, so a normal unchanged-file rescan
won't collapse it back. A deep rescan re-reads tags authoritatively and
will re-create the phantom — documented.
i18n split.{label,help,action} across all 17 locales (count-free action
string to avoid plural machinery). Docs updated in library.md + CLAUDE.md.
InstaZDLL
added
scope: frontend
📝 WalkthroughWalkthroughCette PR ajoute une action « Split this artist » pour convertir un artiste fantôme joint par des virgules en artistes individuels, relier les morceaux concernés, nettoyer l’entrée fantôme et préserver le résultat lors des rescans. ChangesScission des artistes fantômes
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ArtistDetailView
participant ArtistMetadataEditorModal
participant splitArtist
participant split_artist
participant Database
ArtistDetailView->>ArtistMetadataEditorModal: affiche l’artiste fantôme
ArtistMetadataEditorModal->>splitArtist: demande la scission
splitArtist->>split_artist: invoke avec artistId
split_artist->>Database: résout les artistes et réécrit track_artist
Database-->>split_artist: résultat et état du phantom
split_artist-->>splitArtist: SplitArtistResult
splitArtist-->>ArtistMetadataEditorModal: artistes résolus
ArtistMetadataEditorModal->>ArtistDetailView: navigue vers l’artiste principal
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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/crates/app/src/commands/artist_split.rs`:
- Around line 57-214: Ajouter des tests d’intégration avec sqlx::test pour
split_artist couvrant un split simple, la déduplication des fragments, la
conservation du fantôme lorsqu’il reste référencé et la préservation de l’ordre
et des rôles des co-crédits. Vérifier les tables track_artist, track, artist et
le résultat SplitArtistResult après chaque mutation, en réutilisant les fixtures
et helpers de test existants.
In `@src/components/common/ArtistMetadataEditorModal.tsx`:
- Around line 124-126: Update the save-button disabled condition and the guard
in handleSave to use the existing isBusy state, so saving is blocked while
loading, saving, or splitting. Preserve the current save behavior when none of
these operations is active.
In `@src/components/views/ArtistDetailView.tsx`:
- Around line 563-568: Update the onSplit callback in ArtistDetailView so that
when primaryArtistId is null, it navigates to an appropriate fallback artist
instead of remaining on the dissolved artistId. Preserve closing the metadata
editor and the existing navigation to primaryArtistId when it is non-null.
🪄 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: 91165d44-e40d-4cf3-b055-5a09dce1ac7d
📒 Files selected for processing (26)
CLAUDE.mddocs/features/library.mdsrc-tauri/crates/app/src/commands/artist_split.rssrc-tauri/crates/app/src/commands/mod.rssrc-tauri/crates/app/src/commands/scan.rssrc-tauri/crates/app/src/lib.rssrc/components/common/ArtistMetadataEditorModal.tsxsrc/components/views/ArtistDetailView.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/artistOverrides.ts
Comment on lines
+57
to
+214
| pub async fn split_artist( | ||
| state: tauri::State<'_, AppState>, | ||
| artist_id: i64, | ||
| ) -> AppResult<SplitArtistResult> { | ||
| let pool = state.require_profile_pool().await?; | ||
|
|
||
| let name: String = sqlx::query_scalar("SELECT name FROM artist WHERE id = ?") | ||
| .bind(artist_id) | ||
| .fetch_optional(&*pool) | ||
| .await? | ||
| .ok_or_else(|| AppError::Other(format!("artist {artist_id} not found")))?; | ||
|
|
||
| // Comma-split — the deliberate opposite of the scanner's `"; "`-only | ||
| // policy, gated behind this explicit user action so a real name like | ||
| // "Tyler, The Creator" is never fragmented without intent. | ||
| let parts: Vec<String> = name | ||
| .split(',') | ||
| .map(|s| s.trim()) | ||
| .filter(|s| !s.is_empty()) | ||
| .map(|s| s.to_string()) | ||
| .collect(); | ||
| if parts.len() < 2 { | ||
| return Err(AppError::Other( | ||
| "this artist name has no comma-separated parts to split".into(), | ||
| )); | ||
| } | ||
|
|
||
| let mut tx = pool.begin().await?; | ||
|
|
||
| // Resolve each fragment, reusing an existing row by canonical name so | ||
| // a track split off "A, B, C" lands on the already-enriched A / B / C | ||
| // rows; skip the phantom itself in case a fragment canonicalises back | ||
| // to it. | ||
| let mut new_ids: Vec<i64> = Vec::new(); | ||
| for part in &parts { | ||
| if let Some(id) = upsert_artist(&mut tx, part).await? { | ||
| if id != artist_id && !new_ids.contains(&id) { | ||
| new_ids.push(id); | ||
| } | ||
| } | ||
| } | ||
| if new_ids.is_empty() { | ||
| return Err(AppError::Other( | ||
| "splitting this artist produced no distinct artists".into(), | ||
| )); | ||
| } | ||
|
|
||
| // Re-link every track that credits the phantom. Read the whole credit | ||
| // list per track so co-credited artists and their order survive, and | ||
| // expand the phantom in place into the split fragments (role `main`). | ||
| let track_ids: Vec<i64> = | ||
| sqlx::query_scalar("SELECT DISTINCT track_id FROM track_artist WHERE artist_id = ?") | ||
| .bind(artist_id) | ||
| .fetch_all(&mut *tx) | ||
| .await?; | ||
|
|
||
| for tid in &track_ids { | ||
| let rows: Vec<(i64, String)> = sqlx::query_as( | ||
| "SELECT artist_id, role FROM track_artist WHERE track_id = ? ORDER BY position", | ||
| ) | ||
| .bind(tid) | ||
| .fetch_all(&mut *tx) | ||
| .await?; | ||
|
|
||
| let mut rebuilt: Vec<(i64, String)> = Vec::new(); | ||
| let mut seen: std::collections::HashSet<(i64, String)> = std::collections::HashSet::new(); | ||
| for (aid, role) in rows { | ||
| if aid == artist_id { | ||
| for &nid in &new_ids { | ||
| let key = (nid, "main".to_string()); | ||
| if seen.insert(key.clone()) { | ||
| rebuilt.push(key); | ||
| } | ||
| } | ||
| } else if seen.insert((aid, role.clone())) { | ||
| rebuilt.push((aid, role)); | ||
| } | ||
| } | ||
|
|
||
| sqlx::query("DELETE FROM track_artist WHERE track_id = ?") | ||
| .bind(tid) | ||
| .execute(&mut *tx) | ||
| .await?; | ||
| for (position, (aid, role)) in rebuilt.iter().enumerate() { | ||
| sqlx::query( | ||
| "INSERT INTO track_artist (track_id, artist_id, role, position) | ||
| VALUES (?, ?, ?, ?)", | ||
| ) | ||
| .bind(tid) | ||
| .bind(aid) | ||
| .bind(role) | ||
| .bind(position as i64) | ||
| .execute(&mut *tx) | ||
| .await?; | ||
| } | ||
| } | ||
|
|
||
| // Repoint any track whose primary artist was the phantom to the first | ||
| // fragment — matches the scanner's own re-normalisation rule. | ||
| let primary = new_ids[0]; | ||
| sqlx::query("UPDATE track SET primary_artist = ? WHERE primary_artist = ?") | ||
| .bind(primary) | ||
| .bind(artist_id) | ||
| .execute(&mut *tx) | ||
| .await?; | ||
|
|
||
| // Clean up the phantom only when nothing references it any more — | ||
| // guarded on every FK into `artist` so we never SET NULL an album's | ||
| // artist or CASCADE a curated similar list out from under the user. | ||
| let remaining_ta: i64 = | ||
| sqlx::query_scalar("SELECT COUNT(*) FROM track_artist WHERE artist_id = ?") | ||
| .bind(artist_id) | ||
| .fetch_one(&mut *tx) | ||
| .await?; | ||
| let remaining_album: i64 = sqlx::query_scalar("SELECT COUNT(*) FROM album WHERE artist_id = ?") | ||
| .bind(artist_id) | ||
| .fetch_one(&mut *tx) | ||
| .await?; | ||
| let remaining_primary: i64 = | ||
| sqlx::query_scalar("SELECT COUNT(*) FROM track WHERE primary_artist = ?") | ||
| .bind(artist_id) | ||
| .fetch_one(&mut *tx) | ||
| .await?; | ||
| let remaining_curated: i64 = sqlx::query_scalar( | ||
| "SELECT COUNT(*) FROM artist_similar_custom | ||
| WHERE artist_id = ? OR similar_artist_id = ?", | ||
| ) | ||
| .bind(artist_id) | ||
| .bind(artist_id) | ||
| .fetch_one(&mut *tx) | ||
| .await?; | ||
| let phantom_deleted = | ||
| remaining_ta == 0 && remaining_album == 0 && remaining_primary == 0 && remaining_curated == 0; | ||
| if phantom_deleted { | ||
| sqlx::query("DELETE FROM artist WHERE id = ?") | ||
| .bind(artist_id) | ||
| .execute(&mut *tx) | ||
| .await?; | ||
| } | ||
|
|
||
| // Resolve display names for the response before the pool closes. | ||
| let mut artists = Vec::with_capacity(new_ids.len()); | ||
| for &id in &new_ids { | ||
| let nm: String = sqlx::query_scalar("SELECT name FROM artist WHERE id = ?") | ||
| .bind(id) | ||
| .fetch_one(&mut *tx) | ||
| .await?; | ||
| artists.push(SplitArtist { id, name: nm }); | ||
| } | ||
|
|
||
| tx.commit().await?; | ||
|
|
||
| Ok(SplitArtistResult { | ||
| artists, | ||
| tracks_relinked: track_ids.len(), | ||
| phantom_deleted, | ||
| }) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
Aucun test pour cette commande de mutation critique.
split_artist réécrit les crédits de pistes et supprime potentiellement des lignes artist de façon irréversible. Un test d'intégration (au moins : split simple, fragment dupliqué, garde de suppression du fantôme, préservation de l'ordre des co-crédits) sécuriserait ce chemin avant mise en prod.
Veux-tu que je rédige un squelette de tests d'intégration sqlx::test pour couvrir ces cas ?
🤖 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/artist_split.rs` around lines 57 - 214,
Ajouter des tests d’intégration avec sqlx::test pour split_artist couvrant un
split simple, la déduplication des fragments, la conservation du fantôme
lorsqu’il reste référencé et la préservation de l’ordre et des rôles des
co-crédits. Vérifier les tables track_artist, track, artist et le résultat
SplitArtistResult après chaque mutation, en réutilisant les fixtures et helpers
de test existants.
Comment on lines
+124
to
+126
| // initial read is in flight (would clobber local edits), or a save / | ||
| // split is running (would submit stale state / race the relink). | ||
| const isBusy = isLoading || isSaving || isSplitting; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Bloquer aussi la sauvegarde pendant le split.
isBusy inclut bien isSplitting, mais le bouton Save utilise encore isSaving || isLoading et handleSave ne teste que isSaving. L’utilisateur peut donc lancer une sauvegarde concurrente contre l’artiste fantôme pendant que split_artist le relie puis le supprime.
Correctif proposé
const handleSave = async () => {
- if (isSaving) return;
+ if (isBusy) return;
setIsSaving(true);- disabled={isSaving || isLoading}
+ disabled={isBusy}🤖 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/common/ArtistMetadataEditorModal.tsx` around lines 124 - 126,
Update the save-button disabled condition and the guard in handleSave to use the
existing isBusy state, so saving is blocked while loading, saving, or splitting.
Preserve the current save behavior when none of these operations is active.
Comment on lines
+563
to
+568
| onSplit={(primaryArtistId) => { | ||
| // The phantom artist we're viewing was just dissolved (issue | ||
| // #396); jump to the new primary so we don't render a dead id. | ||
| setIsMetadataEditorOpen(false); | ||
| if (primaryArtistId != null) onNavigateToArtist(primaryArtistId); | ||
| }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect relevant symbols
ast-grep outline src/components/views/ArtistDetailView.tsx --view expanded || true
# Show the target region with line numbers
sed -n '520,620p' src/components/views/ArtistDetailView.tsx
# Find the navigate callback and split handler definitions/usages
rg -n "onNavigateToArtist|onSplit\\s*=|primaryArtistId|split" src/components/views/ArtistDetailView.tsx src -g '!**/node_modules/**'Repository: InstaZDLL/WaveFlow
Length of output: 18432
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect navigation helpers and any list/empty-state routes for fallback behavior
rg -n "navigateToArtist|navigate\\(|artistId.*null|empty view|list view|back to.*list|no artist" src/components -g '!**/node_modules/**'
# Inspect the split dialog/modal callback contract, if defined elsewhere
rg -n "onSplit\\s*[:=]|primaryArtistId\\s*[:=]|split artist|dissolved" src -g '!**/node_modules/**'Repository: InstaZDLL/WaveFlow
Length of output: 2987
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '120,180p' src/components/layout/AppLayout.tsx
sed -n '320,420p' src/components/layout/AppLayout.tsx
sed -n '620,690p' src/components/layout/AppLayout.tsxRepository: InstaZDLL/WaveFlow
Length of output: 8892
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' src/components/common/ArtistMetadataEditorModal.tsx
sed -n '180,240p' src/components/common/ArtistMetadataEditorModal.tsxRepository: InstaZDLL/WaveFlow
Length of output: 6126
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '80,120p' src/components/layout/NowPlayingPanel.tsxRepository: InstaZDLL/WaveFlow
Length of output: 1403
Ajouter un fallback quand primaryArtistId vaut null. Le contrat de onSplit autorise ce cas, mais la vue reste alors sur artistId dissous sans navigation de repli ; elle peut donc continuer à afficher/recharger un artiste fantôme.
🤖 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/ArtistDetailView.tsx` around lines 563 - 568, Update the
onSplit callback in ArtistDetailView so that when primaryArtistId is null, it
navigates to an appropriate fallback artist instead of remaining on the
dissolved artistId. Preserve closing the metadata editor and the existing
navigation to primaryArtistId when it is non-null.
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 #396.
Problem
A track whose
ARTISTtag joins contributors with commas (Tibeauthetraveler, Nogymx, Osaki) links to a single phantom artist named after the whole string. The scanner splits on"; "only — deliberately, because a comma can belong to a real name (Tyler, The Creator,Earth, Wind & Fire). So the track dead-ends on a letter-avatar phantom while the real, enriched artists already exist next to it (the album, resolved fromALBUMARTIST, points at the correct row).Approach — option B from the issue (user-driven, no re-tag)
The phantom's Artist Detail → Edit info modal gains a "Split" section, shown only when the name contains a comma. It previews the comma fragments so the user confirms this really is several artists (and not
Tyler, The Creator) before acting. Confirming calls the newsplit_artistcommand, which:track_artistrows (co-credited artists and their order preserved);track.primary_artistwhere it was the phantom;track_artist,album.artist_id,track.primary_artistandartist_similar_custom, so it never SET-NULLs an album artist or CASCADEs a curated similar list.No file is re-tagged. On success the modal navigates to the new primary artist (the phantom page is gone).
Durability across rescans
A pure DB relink would be undone on the next scan: the scanner's skip-branch re-normalises
track_artistwhencurrent_count != splits.len(), and after a split that's3 != 1. Fixed with a matching guard — a comma-joined tag (one"; "-split name) whose track already credits several artists is treated as a deliberate split and left alone. A brand-new scan never hits this (it starts at one phantom row), so it only protects an existing split.Caveat (documented): a deep rescan re-reads tags authoritatively and will re-create the phantom. Re-tagging with
"; "remains the fix that survives that too.Scope
split_artistcommand + registrationonSplitnavigation wiringsplit.{label,help,action}i18n across all 17 locales (count-free action string — no plural machinery)library.md+CLAUDE.mdmulti-artist ruleAlbums-level artists only, matching the issue's scope. Sync emit for the relink is intentionally out of scope for v1 (local library cleanup).
Validation
cargo clippy -p waveflow --all-targets✅bun run typecheck✅bun run lint✅(App-crate unit tests run in CI — local Windows hits the known Tauri-DLL entrypoint issue.)
Summary by CodeRabbit
Nouvelles fonctionnalités
Documentation
Traductions