Propagate album/artist renames across providers#197
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughProviders now emit rename events: ChangesRename propagation (single cohesive DAG)
Sequence DiagramsequenceDiagram
participant Client
participant ArtistProvider
participant ArtistStream as "Artist\nrenamedStream"
participant AlbumProvider
participant AlbumStream as "Album\nrenamedStream"
participant PlayableProvider
Client->>ArtistProvider: update(artist, name: "New Name")
ArtistProvider->>ArtistProvider: detect name change\nupdate fields\nnotifyListeners()
ArtistProvider->>ArtistStream: emit(renamedArtist)
ArtistStream->>AlbumProvider: onArtistRenamed(artist)
AlbumProvider->>AlbumProvider: update cached albums' artistName\nnotifyListeners() if changed
AlbumProvider->>AlbumStream: emit(renamedAlbum) when album renamed
AlbumStream->>PlayableProvider: onAlbumRenamed(album)
PlayableProvider->>PlayableProvider: update songs' albumName\nnotifyListeners() if changed
ArtistStream->>PlayableProvider: onArtistRenamed(artist)
PlayableProvider->>PlayableProvider: update songs' artistName & albumArtistName\nnotifyListeners() if changed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/providers/rename_propagation_test.dart (1)
14-21: 💤 Low valueSimplify the casting chain.
The intermediate casts are unnecessary since
List<Song>is a subtype ofList<Playable>andsyncWithVaultacceptsdynamic.♻️ Proposed simplification
PlayableProvider _seedPlayables(List<Song> songs) { final provider = PlayableProvider(); - provider.syncWithVault(songs.cast<dynamic>().toList() - .map((s) => s as dynamic) - .toList() - .cast<Song>()); + provider.syncWithVault(songs); return provider; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/providers/rename_propagation_test.dart` around lines 14 - 21, The _seedPlayables function contains an unnecessary casting chain when calling PlayableProvider.syncWithVault; remove the intermediate casts and pass the original songs list directly. Locate the _seedPlayables function and replace the provider.syncWithVault(...) call with a direct call using songs (e.g., provider.syncWithVault(songs)) so PlayableProvider and syncWithVault are used without redundant casts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/providers/rename_propagation_test.dart`:
- Around line 14-21: The _seedPlayables function contains an unnecessary casting
chain when calling PlayableProvider.syncWithVault; remove the intermediate casts
and pass the original songs list directly. Locate the _seedPlayables function
and replace the provider.syncWithVault(...) call with a direct call using songs
(e.g., provider.syncWithVault(songs)) so PlayableProvider and syncWithVault are
used without redundant casts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18a1b21a-1a0c-4778-97cf-08abaf1b4355
📒 Files selected for processing (4)
lib/providers/album_provider.dartlib/providers/artist_provider.dartlib/providers/playable_provider.darttest/providers/rename_propagation_test.dart
Songs hold denormalised album/artist names (`albumName`, `artistName`, `albumArtistName`) and albums hold a denormalised `artistName` for performance. When the user edits an album or artist's name, those copies were left stale until the next paginate or relogin — list rows kept showing the old name. Add static broadcast streams `AlbumProvider.renamedStream` and `ArtistProvider.renamedStream` (mirroring the existing `AuthProvider.userLoggedOutStream` pattern). Each `update()` emits on its stream when the server-returned name actually differs. PlayableProvider subscribes to both: - on album rename: songs where `song.albumId == album.id` get `albumName` updated. - on artist rename: songs where `song.artistId == artist.id` get `artistName` updated; songs where `song.albumArtistId == artist.id` get `albumArtistName` updated. The two checks are independent because a featured-track song's primary artist can differ from its album-artist. AlbumProvider also subscribes to `ArtistProvider.renamedStream` and updates `album.artistName` for albums whose `artistId` matches.
phanan
force-pushed
the
feat/propagate-renames-across-providers
branch
from
1afbd84 to
3cee9f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ui/widgets/playable_row.dart`:
- Line 104: The row is being remounted because of key: UniqueKey() in
PlayableRow; remove UniqueKey() and replace it with a stable key so the widget
rebuilds instead of remounting (e.g., use a ValueKey based on the playables
unique identifier or omit the key entirely) to preserve PlayableRowThumbnail and
its stream subscriptions during PlayableProvider updates; update the
instantiation in PlayableRow to use a stable key (for example
ValueKey(playable.id) or similar stable identifier) rather than UniqueKey().
In `@test/ui/widgets/song_card_test.dart`:
- Around line 16-28: The PlayableProvider created in _harness is never disposed
and leaks stream listeners; fix by registering disposal with the test harness:
after creating provider in _harness(WidgetTester tester, Playable playable) call
tester.addTearDown(() => provider.dispose()); this ensures the PlayableProvider
instance used by SongCard is cleaned up between tests (alternatively ensure the
returned _Harness disposes the provider in its dispose method if you prefer
encapsulation).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c4cb82c-72be-4f3f-8e68-7bf97350957d
📒 Files selected for processing (3)
lib/ui/widgets/playable_row.dartlib/ui/widgets/song_card.darttest/ui/widgets/song_card_test.dart
Earlier patch only wrapped SongCard / PlayableRow. Top Albums and Top Artists on the home screen, the album/artist details headers, and the now-playing subtitle were still reading mutable name fields without observing the provider, so they kept showing the pre-rename value. Wraps AlbumCard, ArtistCard, PlayableInfo, and the album/artist details FutureBuilders in the matching Consumer. Updates the card tests to provide the right ChangeNotifierProvider and add a rebuild- on-notify case for each.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/ui/widgets/now_playing/playable_info.dart (2)
12-16: 💤 Low valueUse a local variable to enable type promotion and drop the redundant explicit casts.
Because
playableis an instance field, Dart cannot promote its type after anischeck, requiring the redundantascast on every branch. Assigning to a local variable first gives the compiler enough information to smart-cast.♻️ Suggested refactor
String get _subtitle { - if (playable is Song) return (playable as Song).artistName; - if (playable is Episode) return (playable as Episode).podcastTitle; + final p = playable; + if (p is Song) return p.artistName; + if (p is Episode) return p.podcastTitle; return ''; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/widgets/now_playing/playable_info.dart` around lines 12 - 16, The _subtitle getter uses `playable` (an instance field) with `is` checks but still uses redundant `as` casts; to enable type promotion and remove casts, assign `playable` to a local final variable (e.g., `final p = playable`) at the start of the `_subtitle` getter and then use `if (p is Song) return p.artistName; if (p is Episode) return p.podcastTitle;` so the compiler can smart-cast and the explicit `as` casts can be removed.
21-21: 💤 Low valuePrefer descriptive names over
_/__/___for Consumer callback params.All three parameters are discarded, but the learning for this repo explicitly calls out callback params as needing names that say what they are. Naming them even while unused makes the signature self-documenting and consistent with the project style.
♻️ Suggested rename
- builder: (_, __, ___) => Column( + builder: (context, provider, child) => Column(Based on learnings: "For callback params, destructured fields, lambda args, and other uses, pick a name that says what it is."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/widgets/now_playing/playable_info.dart` at line 21, The Consumer builder callback currently uses `_`, `__`, `___`; rename these to descriptive parameter names that match the Consumer signature (e.g., `context`, `value`, `child` or `context`, `watch`, `child` depending on which provider API is used) in the `builder: (_, __, ___) => Column(...)` expression inside playable_info.dart so the signature is self-documenting and consistent with project style; update any internal references if used and leave unused params named (not `_`) to document their roles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/ui/widgets/now_playing/playable_info.dart`:
- Around line 12-16: The _subtitle getter uses `playable` (an instance field)
with `is` checks but still uses redundant `as` casts; to enable type promotion
and remove casts, assign `playable` to a local final variable (e.g., `final p =
playable`) at the start of the `_subtitle` getter and then use `if (p is Song)
return p.artistName; if (p is Episode) return p.podcastTitle;` so the compiler
can smart-cast and the explicit `as` casts can be removed.
- Line 21: The Consumer builder callback currently uses `_`, `__`, `___`; rename
these to descriptive parameter names that match the Consumer signature (e.g.,
`context`, `value`, `child` or `context`, `watch`, `child` depending on which
provider API is used) in the `builder: (_, __, ___) => Column(...)` expression
inside playable_info.dart so the signature is self-documenting and consistent
with project style; update any internal references if used and leave unused
params named (not `_`) to document their roles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fd10176-e280-4cbe-8901-2a91748b5af3
📒 Files selected for processing (7)
lib/ui/screens/album_details.dartlib/ui/screens/artist_details.dartlib/ui/widgets/album_card.dartlib/ui/widgets/artist_card.dartlib/ui/widgets/now_playing/playable_info.darttest/ui/widgets/album_card_test.darttest/ui/widgets/artist_card_test.dart
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.
Summary
Songs hold denormalised album/artist names (
albumName,artistName,albumArtistName) and albums hold a denormalisedartistNamefor performance. When the user edits an album or artist's name, those copies were left stale until the next paginate or relogin — list rows kept showing the old name.This PR fans the rename out across providers via two new static broadcast streams that mirror the existing
AuthProvider.userLoggedOutStreampattern:AlbumProvider.renamedStream— fires fromupdate()when the server-returned name differs.ArtistProvider.renamedStream— same shape.PlayableProvidersubscribes to both:song.albumId == album.idgetalbumNameupdated.song.artistId == artist.idgetartistNameupdated; songs wheresong.albumArtistId == artist.idgetalbumArtistNameupdated. The two checks are independent because a featured-track song's primary artist can differ from its album-artist.AlbumProvideralso subscribes toArtistProvider.renamedStreamand updatesalbum.artistNamefor albums whoseartistIdmatches.Test plan
flutter test— all 380 tests pass, includingtest/providers/rename_propagation_test.dart(5 new):Summary by CodeRabbit
Bug Fixes
Refactor
Documentation
Tests