Remove write operations that require ACL#169
Merged
Merged
Conversation
Without user ACL support in the app, write operations on shared resources can fail or cause unintended side effects. This removes: - Radio stations: add, edit, delete (now read-only + play) - Podcasts: add, unsubscribe (now read-only + play) - Playlists: edit, delete, remove songs (keep create + queue actions) - Playlist folders: rename, delete (keep expand/collapse + queue)
|
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 (1)
📝 WalkthroughWalkthroughRemoved long-press action menus, per-item swipe-to-delete and deletion confirmation flows across playlist_details, playlists, podcasts, and radio screens; dismiss gestures now only queue items (never remove). Navigation add buttons and empty-state add actions were removed or simplified; radio screen added a local Changes
Sequence Diagram(s)mermaid mermaid 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
lib/ui/screens/podcasts.dart (1)
198-199: Non-final fields inStatelessWidgetare a code smell.
currentFieldandcurrentOrderare mutable butPodcastSortButtonis aStatelessWidget. While the mutation happens only inonSelected, this violates the immutability contract ofStatelessWidgetand could lead to subtle bugs if the widget is reused.💡 Proposed fix: make fields final and track state externally
class PodcastSortButton extends StatelessWidget { final void Function(PodcastSortConfig sortConfig)? onMenuItemSelected; - PodcastSortField currentField; - SortOrder currentOrder; + final PodcastSortField currentField; + final SortOrder currentOrder;Then update
onSelectedto compute the new order and pass it to the callback, letting the parent manage state (which it already does viasetState).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/podcasts.dart` around lines 198 - 199, PodcastSortButton currently declares non-final mutable fields currentField and currentOrder inside a StatelessWidget; make both fields final and remove any mutations from the widget itself, then update the onSelected handler to compute the new PodcastSortField and SortOrder and invoke the provided callback with those values so the parent owns state updates (the parent already calls setState). Specifically: change currentField and currentOrder to final in PodcastSortButton, stop assigning to them inside onSelected, compute newOrder/newField locally in onSelected and pass them out via the existing callback so the parent updates its state.lib/ui/screens/radio_stations.dart (1)
33-38: Consider adding user feedback for fetch errors.The catch block silently swallows errors without notifying the user. While this may be intentional, users won't know if fetching failed—they'll just see an empty list or stale data.
💡 Optional: Add error state handling
Future<void> _fetchData() async { if (_loading) return; setState(() => _loading = true); try { await context.read<RadioStationProvider>().fetchAll(); } catch (_) { + if (mounted) { + showOverlay( + context, + caption: 'Error', + message: 'Could not load stations.', + icon: CupertinoIcons.exclamationmark_triangle, + ); + } } finally { if (mounted) setState(() => _loading = false); } }Note: The podcasts screen tracks
_erroredstate and showsOopsBoxon failure—consider consistency across screens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/radio_stations.dart` around lines 33 - 38, The catch block swallowing errors in the widget leaves users uninformed; update the error handling in the try/catch around context.read<RadioStationProvider>().fetchAll() to set an error state (e.g. setState(() => _errored = true)) and optionally log the exception, ensure you still clear _loading in the finally block, and then update the widget tree to display the same OopsBox used by the podcasts screen when _errored is true so failures show user-visible feedback; reference RadioStationProvider.fetchAll, the _loading/_errored state variables, mounted and OopsBox when making the changes.lib/ui/screens/playlists.dart (2)
104-120: Consider parallelizing playlist fetches for better performance.Sequential
awaitinside the loop (lines 108-110) fetches playlists one-by-one, which can be slow for folders with many playlists. Also, if anyfetchForPlaylistcall fails, the entire operation fails without partial feedback.💡 Proposed fix: fetch in parallel, then queue sequentially
confirmDismiss: (_) async { final playableProvider = context.read<PlayableProvider>(); - final songs = <Playable>[]; - for (final p in folderPlaylists) { - songs.addAll(await playableProvider.fetchForPlaylist(p.id)); - } + final results = await Future.wait( + folderPlaylists.map((p) => playableProvider.fetchForPlaylist(p.id)), + ); + final songs = results.expand((s) => s).toList(); if (songs.isNotEmpty) { for (final song in songs) { await audioHandler.queueToBottom(song); }Note: Sequential queueing (
queueToBottom) is likely intentional for ordering, but the fetch can be parallelized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/playlists.dart` around lines 104 - 120, The loop in confirmDismiss uses sequential awaits on playableProvider.fetchForPlaylist for each playlist in folderPlaylists which is slow and brittle; change it to kick off all fetches in parallel (map folderPlaylists to futures and use Future.wait) and collect/flatten successful results, handling individual fetch failures (try/catch per playlist future or filter out null/failed results) so one failure doesn't abort all; once you have the flattened songs list, keep sequential calls to audioHandler.queueToBottom to preserve order and then call showOverlay with either 'Queued' or 'No songs found.' based on the final list.
250-250: Use a stable key forDismissible.
ValueKey(playlist)relies onPlaylist's equality implementation. IfPlaylistdoesn't override==andhashCode, the key is based on object identity, which can cause issues when the list is rebuilt with new instances. Consider usingValueKey(playlist.id)for stability.💡 Proposed fix
- key: ValueKey(playlist), + key: ValueKey(playlist.id),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/playlists.dart` at line 250, The Dismissible is using unstable ValueKey(playlist) which depends on Playlist's identity; change it to use a stable identifier like ValueKey(playlist.id) so rebuilds with new Playlist instances keep consistent keys; update the Dismissible key in lib/ui/screens/playlists.dart (where ValueKey(playlist) is set) and ensure the Playlist model exposes a stable id property (e.g., id or uid) to use instead.
🤖 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/screens/podcasts.dart`:
- Around line 95-100: The current code returns a Card that wraps PodcastRow,
causing double Card nesting because PodcastRow already wraps its contents in a
Card; remove the outer Card wrapper so the build returns PodcastRow(podcast:
podcast, router: widget.router) directly (locate the return that constructs Card
around PodcastRow and replace it with returning the PodcastRow instance).
---
Nitpick comments:
In `@lib/ui/screens/playlists.dart`:
- Around line 104-120: The loop in confirmDismiss uses sequential awaits on
playableProvider.fetchForPlaylist for each playlist in folderPlaylists which is
slow and brittle; change it to kick off all fetches in parallel (map
folderPlaylists to futures and use Future.wait) and collect/flatten successful
results, handling individual fetch failures (try/catch per playlist future or
filter out null/failed results) so one failure doesn't abort all; once you have
the flattened songs list, keep sequential calls to audioHandler.queueToBottom to
preserve order and then call showOverlay with either 'Queued' or 'No songs
found.' based on the final list.
- Line 250: The Dismissible is using unstable ValueKey(playlist) which depends
on Playlist's identity; change it to use a stable identifier like
ValueKey(playlist.id) so rebuilds with new Playlist instances keep consistent
keys; update the Dismissible key in lib/ui/screens/playlists.dart (where
ValueKey(playlist) is set) and ensure the Playlist model exposes a stable id
property (e.g., id or uid) to use instead.
In `@lib/ui/screens/podcasts.dart`:
- Around line 198-199: PodcastSortButton currently declares non-final mutable
fields currentField and currentOrder inside a StatelessWidget; make both fields
final and remove any mutations from the widget itself, then update the
onSelected handler to compute the new PodcastSortField and SortOrder and invoke
the provided callback with those values so the parent owns state updates (the
parent already calls setState). Specifically: change currentField and
currentOrder to final in PodcastSortButton, stop assigning to them inside
onSelected, compute newOrder/newField locally in onSelected and pass them out
via the existing callback so the parent updates its state.
In `@lib/ui/screens/radio_stations.dart`:
- Around line 33-38: The catch block swallowing errors in the widget leaves
users uninformed; update the error handling in the try/catch around
context.read<RadioStationProvider>().fetchAll() to set an error state (e.g.
setState(() => _errored = true)) and optionally log the exception, ensure you
still clear _loading in the finally block, and then update the widget tree to
display the same OopsBox used by the podcasts screen when _errored is true so
failures show user-visible feedback; reference RadioStationProvider.fetchAll,
the _loading/_errored state variables, mounted and OopsBox when making the
changes.
🪄 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: cd7052d2-979d-4d55-aee3-bca599074ca7
📒 Files selected for processing (4)
lib/ui/screens/playlist_details.dartlib/ui/screens/playlists.dartlib/ui/screens/podcasts.dartlib/ui/screens/radio_stations.dart
💤 Files with no reviewable changes (1)
- lib/ui/screens/playlist_details.dart
- Remove outer Card wrapper around PodcastRow (was double-nested) - Make PodcastSortButton fields final, compute sort state locally - Parallelize folder playlist fetches with Future.wait - Use ValueKey(playlist.id) instead of unstable ValueKey(playlist) - Add error state and OopsBox to radio stations screen
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/screens/podcasts.dart`:
- Around line 81-85: The empty-state branch for podcasts (inside the
podcasts.isEmpty check in podcasts.dart) fails to include navigationBar!,
causing the header/title to disappear; update that branch so widgets includes
navigationBar! first and then the SliverToBoxAdapter(child: NoPodcastsScreen()),
i.e., push navigationBar! into the widgets list before rendering the empty
NoPodcastsScreen so the header remains visible.
In `@lib/ui/screens/radio_stations.dart`:
- Around line 54-57: The early returns when provider.stations.isEmpty (checking
_loading and _errored and returning Spinner or OopsBox) remove the
CustomScrollView and thus the CupertinoSliverNavigationBar; instead, remove
those early returns and render the loading/error placeholders as slivers inside
the existing sliver tree so the navigation bar remains present — e.g., inside
the CustomScrollView include a SliverToBoxAdapter (or similar) that shows
Spinner when _loading and OopsBox (wired to _fetchData) when _errored, while
keeping the list rendering logic for provider.stations unchanged.
🪄 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: 98aa1ef8-b4d0-41b5-a088-df37bbaba01d
📒 Files selected for processing (3)
lib/ui/screens/playlists.dartlib/ui/screens/podcasts.dartlib/ui/screens/radio_stations.dart
✅ Files skipped from review due to trivial changes (1)
- lib/ui/screens/playlists.dart
The header disappeared when the podcast list was empty because navigationBar was not included in the widgets list.
The early returns for loading/error states bypassed the CustomScrollView, losing the navigation bar. Now they render as slivers within the scroll view.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/screens/podcasts.dart`:
- Around line 83-85: The empty-state sliver is wrapped in SliverToBoxAdapter
causing the NoPodcastsScreen to be measured as full-screen and create a scroll
gap under navigationBar; replace the SliverToBoxAdapter(s) that wrap
NoPodcastsScreen with SliverFillRemaining (or SliverFillRemaining(hasScrollBody:
false) if needed) so the empty state fills remaining viewport space correctly;
update both occurrences that wrap NoPodcastsScreen (the instance near
navigationBar and the one referenced around the 169-171 region) to use
SliverFillRemaining to remove the extra scrollable gap.
🪄 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: 7d7f0fef-38ec-48f3-af2b-3f0291f9e93b
📒 Files selected for processing (2)
lib/ui/screens/podcasts.dartlib/ui/screens/radio_stations.dart
SliverToBoxAdapter with a full-screen-height container created excessive scroll below the nav bar. SliverFillRemaining correctly fills the remaining viewport.
This was referenced May 1, 2026
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
Test plan
Summary by CodeRabbit
Changes
Refactor