fix(artist): split conjunctions correctly and resolve missing artist IDs#3813
Draft
kairosci wants to merge 3 commits into
Draft
fix(artist): split conjunctions correctly and resolve missing artist IDs#3813kairosci wants to merge 3 commits into
kairosci wants to merge 3 commits into
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR replaces hardcoded artist separator strings throughout the app with a shared, localization-aware ChangesArtist Separator Standardization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
This applies to Italian artists, but the same principle applies to artists of other nationalities as well |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
innertube/src/main/kotlin/com/metrolist/innertube/models/Runs.kt (1)
34-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake conjunction splitting case-insensitive
conjunctionPatternis built withoutRegexOption.IGNORE_CASE, so in-text splitting remains case-sensitive while the standalone conjunction check is case-insensitive—mixed/upper-case conjunctions can bypass splitting.Proposed fix
- val conjunctionPattern = Regex( - if (words.isNotEmpty()) " (${words.joinToString("|") { Regex.escape(it) }}) | & " - else " & " - ) + val conjunctionPattern = Regex( + if (words.isNotEmpty()) " (${words.joinToString("|") { Regex.escape(it) }}) | & " + else " & ", + options = setOf(RegexOption.IGNORE_CASE) + )🤖 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 `@innertube/src/main/kotlin/com/metrolist/innertube/models/Runs.kt` around lines 34 - 37, conjunctionPattern in Runs.kt is constructed without case-insensitive matching so mixed/upper-case conjunctions aren't split; update the Regex creation for conjunctionPattern to use case-insensitive matching (e.g., pass RegexOption.IGNORE_CASE or embed (?i)) so the pattern built from words and the standalone " & " are matched regardless of case, keeping the same joinToString logic that escapes words.
🤖 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 `@app/src/main/kotlin/com/metrolist/music/db/entities/SpeedDialItem.kt`:
- Line 80: Deserialization is still splitting artist strings using the
hard-coded ", " while serialization uses ARTIST_SEPARATOR (causing round-trip
failure); update the deserialization calls that currently use split(", ") to
split(ARTIST_SEPARATOR) (and preserve trimming if needed), i.e., change the code
that converts the subtitle back into artist names to use ARTIST_SEPARATOR
instead of the literal ", " in the SpeedDialItem deserialization logic so both
occurrences are fixed.
In `@app/src/main/kotlin/com/metrolist/music/ui/player/MiniPlayer.kt`:
- Around line 674-677: In MiniPlayer.kt update the logic that computes
displayArtists: stop filtering artists by it.id != null and instead only filter
by non-blank names (e.g., metadata.artists.filter { it.name.isNotBlank() }) so
the Text that joins names with ARTIST_SEPARATOR always shows valid names;
preserve any existing id checks where artist IDs are required for actions
(subscribe/navigation) rather than for rendering.
In `@app/src/main/kotlin/com/metrolist/music/utils/PlaylistExporter.kt`:
- Line 133: PlaylistExporter started using the ARTIST_SEPARATOR constant when
serializing artists but loadM3UOnline in BackupRestoreViewModel still splits
artist fields with a hardcoded ';', breaking round-trip imports; update
loadM3UOnline to use the same ARTIST_SEPARATOR constant (or split on
Regex.escape(ARTIST_SEPARATOR) if treating it as a regex) instead of split(';')
so both export (PlaylistExporter) and import (loadM3UOnline) use the identical
delimiter.
In `@app/src/main/kotlin/com/metrolist/music/utils/Utils.kt`:
- Line 12: Replace the hardcoded ARTIST_SEPARATOR constant with a locale-aware
retrieval and update callsites to use it: remove or stop using const val
ARTIST_SEPARATOR in Utils.kt and provide a way to get the separator from
resources (e.g., use context.getString(R.string.and) or
stringResource(R.string.and) in composables) so UI code that composes artists
(where ARTIST_SEPARATOR was used) builds the separator as "
${context.getString(R.string.and)} " rather than the literal " e "; update all
usages to call the new localized accessor or inline resource lookup (referencing
R.string.and and the former ARTIST_SEPARATOR symbol to find replacements).
---
Outside diff comments:
In `@innertube/src/main/kotlin/com/metrolist/innertube/models/Runs.kt`:
- Around line 34-37: conjunctionPattern in Runs.kt is constructed without
case-insensitive matching so mixed/upper-case conjunctions aren't split; update
the Regex creation for conjunctionPattern to use case-insensitive matching
(e.g., pass RegexOption.IGNORE_CASE or embed (?i)) so the pattern built from
words and the standalone " & " are matched regardless of case, keeping the same
joinToString logic that escapes words.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39b94326-94e9-4d2d-a7cf-c9f6c8859fff
📒 Files selected for processing (20)
app/src/main/kotlin/com/metrolist/music/db/entities/SpeedDialItem.ktapp/src/main/kotlin/com/metrolist/music/listentogether/ListenTogetherManager.ktapp/src/main/kotlin/com/metrolist/music/playback/MediaLibrarySessionCallback.ktapp/src/main/kotlin/com/metrolist/music/playback/MusicService.ktapp/src/main/kotlin/com/metrolist/music/ui/component/Items.ktapp/src/main/kotlin/com/metrolist/music/ui/component/SongDropdownSelect.ktapp/src/main/kotlin/com/metrolist/music/ui/menu/AlbumMenu.ktapp/src/main/kotlin/com/metrolist/music/ui/menu/SongMenu.ktapp/src/main/kotlin/com/metrolist/music/ui/menu/YouTubeSongMenu.ktapp/src/main/kotlin/com/metrolist/music/ui/player/MiniPlayer.ktapp/src/main/kotlin/com/metrolist/music/ui/player/Player.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/HomeScreen.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/wrapped/pages/WrappedTop5SongsScreen.ktapp/src/main/kotlin/com/metrolist/music/utils/PlaylistExporter.ktapp/src/main/kotlin/com/metrolist/music/utils/Utils.ktapp/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.ktapp/src/main/res/values-it/metrolist_strings.xmlinnertube/src/main/kotlin/com/metrolist/innertube/models/Runs.ktinnertube/src/main/kotlin/com/metrolist/innertube/pages/ArtistItemsPage.ktinnertube/src/main/kotlin/com/metrolist/innertube/pages/ArtistPage.kt
Comment on lines
+674
to
+677
| val displayArtists = metadata.artists.filter { it.id != null && it.name.isNotBlank() } | ||
| if (displayArtists.isNotEmpty()) { | ||
| Text( | ||
| text = metadata.artists.joinToString { it.name }, | ||
| text = displayArtists.joinToString(ARTIST_SEPARATOR) { it.name }, |
Contributor
There was a problem hiding this comment.
Do not gate displayed artist names on id.
At Line 674, filtering by it.id != null can hide valid artist names in the mini-player subtitle when IDs are unavailable. For text rendering, filter by non-blank name only; keep ID checks for actions (e.g., subscribe).
Proposed fix
- val displayArtists = metadata.artists.filter { it.id != null && it.name.isNotBlank() }
+ val displayArtists = metadata.artists.filter { it.name.isNotBlank() }
if (displayArtists.isNotEmpty()) {
Text(
text = displayArtists.joinToString(ARTIST_SEPARATOR) { it.name },🤖 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 `@app/src/main/kotlin/com/metrolist/music/ui/player/MiniPlayer.kt` around lines
674 - 677, In MiniPlayer.kt update the logic that computes displayArtists: stop
filtering artists by it.id != null and instead only filter by non-blank names
(e.g., metadata.artists.filter { it.name.isNotBlank() }) so the Text that joins
names with ARTIST_SEPARATOR always shows valid names; preserve any existing id
checks where artist IDs are required for actions (subscribe/navigation) rather
than for rendering.
kairosci
force-pushed
the
fix/artist-conjunction-handling
branch
from
57b35a7 to
b83719f
Compare
kairosci
force-pushed
the
fix/artist-conjunction-handling
branch
from
b83719f to
08a5f17
Compare
When artists are separated by conjunctions in YouTube Music API responses, all artists now maintain their navigationEndpoint, not just the first one. This allows each artist to be individually clickable without showing commas. Fixes: PR MetrolistGroup#3813
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.
Problem
Artist names joined by conjunctions (e.g., "Tiziano Ferro e Ariete") were not split correctly. Conjunction words like "e" (Italian for "and") appeared as separate clickable artists, and when splitting a single run, the second artist inherited the first artist's navigation endpoint, leading to wrong artist pages.
Cause
splitArtistsByConjunction()only checked for standalone conjunction runs but did not split runs containing conjunction wordsnavigationEndpoint, making the second artist navigate to the first artist's pagemapNotNullinfromMusicTwoRowItemRendererfiltered out artists withnullIDs beforeresolveArtistIds()could resolve themSolution
navigationEndpoint; subsequent parts getnullmapNotNullwith.mapinfromMusicTwoRowItemRendererso null-ID artists are preserved forresolveArtistIds()ARTIST_SEPARATORconstant (" e ")Testing
./gradlew :app:assembleFossDebugSummary by CodeRabbit
Release Notes