Full-width background image in app bar headers#157
Merged
Conversation
Remove the centered cover image from the app bar header. The background image now fills the full width and height with no blur, fading to transparent at the bottom via ShaderMask. Remove unused CoverImageStack and CoverImage classes and clean up dead cover image code from all detail screens.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRemoved AppBar's Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Add backgroundImageFromPlayables helper that picks the first playable with a custom image. Applied to: playlists without cover, favorites, recently played, all songs, downloaded, queue, and genre details. Playlists with an explicit cover still use that cover.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
lib/ui/screens/artist_details.dart (1)
87-98:⚠️ Potential issue | 🟡 MinorAdd Hero widget wrapper to restore artist-details transition animation.
The code emits
artist-hero-*tags from source widgets but this screen lacks a matching Hero destination, causing the navigation animation to regress. Wrap the background image in a Hero widget using the artist's ID:Suggested fix
Hero( tag: 'artist-hero-${artist.id}', child: SizedBox.expand( child: DecoratedBox( decoration: BoxDecoration( image: DecorationImage( image: artist.image, fit: BoxFit.cover, alignment: Alignment.topCenter, ), ), ), ), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/artist_details.dart` around lines 87 - 98, Wrap the background image DecoratedBox in a Hero widget so the destination matches the source hero tags (use the tag 'artist-hero-${artist.id}'); locate the backgroundImage child in the artist details screen (the SizedBox.expand/DecoratedBox block) and replace that child with a Hero whose tag is built from artist.id and whose child is the existing SizedBox.expand containing the DecoratedBox to restore the transition animation.lib/ui/screens/podcast_details.dart (1)
89-100:⚠️ Potential issue | 🟡 MinorAdd missing Hero destination for podcast image transition.
The podcast card sources a Hero animation with tag
podcast-hero-${podcast.id}(viaAlbumArtistThumbnailwithasHero: true), but the destination screen has no matching Hero wrapper around the image. Wrap the podcast image inpodcast_details.dartline 89 with a Hero widget to complete the transition:Hero( tag: 'podcast-hero-${podcast.id}', child: SizedBox.expand( child: DecoratedBox( decoration: BoxDecoration( image: DecorationImage( image: podcast.image, fit: BoxFit.cover, alignment: Alignment.topCenter, ), ), ), ), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/podcast_details.dart` around lines 89 - 100, The podcast image background lacks a matching Hero destination for the source Hero tag used by AlbumArtistThumbnail (asHero) — wrap the SizedBox.expand > DecoratedBox that uses podcast.image with a Hero whose tag is 'podcast-hero-${podcast.id}' so the transition completes; locate the backgroundImage builder that creates SizedBox.expand/DecoratedBox and replace the child with a Hero(tag: 'podcast-hero-${podcast.id}', child: the existing SizedBox.expand child) ensuring podcast.id and podcast.image are referenced in the tag and decoration respectively.lib/ui/screens/album_details.dart (1)
85-96:⚠️ Potential issue | 🟡 MinorAdd Hero widget to match album thumbnail animation source.
Album thumbnails use
Hero(tag: 'album-hero-${album.id}')for shared-axis transitions, but the destination header at lines 85–96 lacks a matching Hero wrapper. The background image should be wrapped inHero(tag: 'album-hero-${album.id}')to preserve the animation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/album_details.dart` around lines 85 - 96, Wrap the header background image widget in a Hero using the same tag as the thumbnail so the shared-axis transition works: wrap the existing SizedBox.expand / DecoratedBox block in Hero(tag: 'album-hero-${album.id}', child: ...). Locate the header backgroundImage construction in album_details.dart (the SizedBox.expand -> DecoratedBox block) and replace it with a Hero whose child is the same DecoratedBox; ensure the tag uses album.id exactly as the source thumbnails do.
🧹 Nitpick comments (1)
test/ui/widgets/app_bar_test.dart (1)
7-21: Consider adding a regression assert for “no foreground cover” in this test.This case now validates only text, so it won’t catch accidental reintroduction of a centered cover element.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/widgets/app_bar_test.dart` around lines 7 - 21, The test 'renders heading text' currently only asserts the heading string; add a regression assert that no foreground cover is present by asserting the cover widget is absent when rendering koel.AppBar with headingText — e.g. add an expectation that find.byKey(ValueKey('foregroundCover')) (or the actual Key/Type used for the centered cover in your AppBar implementation) yields findsNothing so accidental reintroduction of the centered cover will fail the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/ui/screens/album_details.dart`:
- Around line 85-96: Wrap the header background image widget in a Hero using the
same tag as the thumbnail so the shared-axis transition works: wrap the existing
SizedBox.expand / DecoratedBox block in Hero(tag: 'album-hero-${album.id}',
child: ...). Locate the header backgroundImage construction in
album_details.dart (the SizedBox.expand -> DecoratedBox block) and replace it
with a Hero whose child is the same DecoratedBox; ensure the tag uses album.id
exactly as the source thumbnails do.
In `@lib/ui/screens/artist_details.dart`:
- Around line 87-98: Wrap the background image DecoratedBox in a Hero widget so
the destination matches the source hero tags (use the tag
'artist-hero-${artist.id}'); locate the backgroundImage child in the artist
details screen (the SizedBox.expand/DecoratedBox block) and replace that child
with a Hero whose tag is built from artist.id and whose child is the existing
SizedBox.expand containing the DecoratedBox to restore the transition animation.
In `@lib/ui/screens/podcast_details.dart`:
- Around line 89-100: The podcast image background lacks a matching Hero
destination for the source Hero tag used by AlbumArtistThumbnail (asHero) — wrap
the SizedBox.expand > DecoratedBox that uses podcast.image with a Hero whose tag
is 'podcast-hero-${podcast.id}' so the transition completes; locate the
backgroundImage builder that creates SizedBox.expand/DecoratedBox and replace
the child with a Hero(tag: 'podcast-hero-${podcast.id}', child: the existing
SizedBox.expand child) ensuring podcast.id and podcast.image are referenced in
the tag and decoration respectively.
---
Nitpick comments:
In `@test/ui/widgets/app_bar_test.dart`:
- Around line 7-21: The test 'renders heading text' currently only asserts the
heading string; add a regression assert that no foreground cover is present by
asserting the cover widget is absent when rendering koel.AppBar with headingText
— e.g. add an expectation that find.byKey(ValueKey('foregroundCover')) (or the
actual Key/Type used for the centered cover in your AppBar implementation)
yields findsNothing so accidental reintroduction of the centered cover will fail
the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f71103a-10de-4405-95ea-22e455c176f9
📒 Files selected for processing (12)
lib/ui/screens/album_details.dartlib/ui/screens/artist_details.dartlib/ui/screens/downloaded.dartlib/ui/screens/favorites.dartlib/ui/screens/genre_details.dartlib/ui/screens/playlist_details.dartlib/ui/screens/podcast_details.dartlib/ui/screens/queue.dartlib/ui/screens/recently_played.dartlib/ui/screens/songs.dartlib/ui/widgets/app_bar.darttest/ui/widgets/app_bar_test.dart
💤 Files with no reviewable changes (6)
- lib/ui/screens/queue.dart
- lib/ui/screens/favorites.dart
- lib/ui/screens/genre_details.dart
- lib/ui/screens/songs.dart
- lib/ui/screens/recently_played.dart
- lib/ui/screens/downloaded.dart
Apply a color matrix filter (1.2x contrast, -20 brightness) to header background images, making edges sharper and low-resolution images look crisper.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/ui/screens/downloaded.dart (1)
93-94: Consider caching the background image to avoid repeated computation on each build.
backgroundImageFromPlayables()iterates through the entire playables list and allocates new widget objects on every call. Sincebuild()runs on eachsetState(search query changes, sort changes), this introduces O(n) scans and allocations per rebuild.The previous implementation used a cached
_coverstate field with lazy initialization. Consider restoring a similar caching pattern:♻️ Suggested caching approach
class _DownloadedScreenState extends State<DownloadedScreen> { var _searchQuery = ''; + Widget? _backgroundImage; `@override` Widget build(BuildContext context) { ... return CustomScrollView( slivers: <Widget>[ AppBar( headingText: 'Downloaded', - backgroundImage: backgroundImageFromPlayables( - provider.playables), + backgroundImage: _backgroundImage ??= + backgroundImageFromPlayables(provider.playables),Note: You may need to invalidate
_backgroundImagewhen the playables list changes if the provider can update independently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/downloaded.dart` around lines 93 - 94, The build currently calls backgroundImageFromPlayables(provider.playables) on every rebuild which re-scans playables and allocates widgets; replace that with a cached state field (e.g. _backgroundImage) and lazy-initialize it (compute once from backgroundImageFromPlayables when null) and assign it in initState or the first build; invalidate and recompute _backgroundImage whenever provider.playables changes (use didUpdateWidget, a provider listener, or compare list identity/length in setState) so updates still reflect new playables, and then return the cached _backgroundImage from build instead of calling backgroundImageFromPlayables each time.
🤖 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/screens/downloaded.dart`:
- Around line 93-94: The build currently calls
backgroundImageFromPlayables(provider.playables) on every rebuild which re-scans
playables and allocates widgets; replace that with a cached state field (e.g.
_backgroundImage) and lazy-initialize it (compute once from
backgroundImageFromPlayables when null) and assign it in initState or the first
build; invalidate and recompute _backgroundImage whenever provider.playables
changes (use didUpdateWidget, a provider listener, or compare list
identity/length in setState) so updates still reflect new playables, and then
return the cached _backgroundImage from build instead of calling
backgroundImageFromPlayables each time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 651b6c47-1f7b-40e5-a8fe-845ced42ba0c
📒 Files selected for processing (11)
lib/router.dartlib/ui/screens/downloaded.dartlib/ui/screens/favorites.dartlib/ui/screens/genre_details.dartlib/ui/screens/library.dartlib/ui/screens/playlist_details.dartlib/ui/screens/queue.dartlib/ui/screens/recently_played.dartlib/ui/screens/screens.dartlib/ui/screens/songs.dartlib/ui/widgets/app_bar.dart
💤 Files with no reviewable changes (3)
- lib/ui/screens/screens.dart
- lib/router.dart
- lib/ui/screens/queue.dart
✅ Files skipped from review due to trivial changes (3)
- lib/ui/screens/genre_details.dart
- lib/ui/screens/songs.dart
- lib/ui/screens/favorites.dart
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/ui/screens/recently_played.dart
- lib/ui/screens/playlist_details.dart
- lib/ui/widgets/app_bar.dart
Wrap background images in Hero widgets with matching tags so the shared-axis transition from thumbnail to detail screen works.
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
CoverImageStackandCoverImageclassesTest plan
flutter testSummary by CodeRabbit
Style
Chores
Tests