Replace podcast popup menu with a full action sheet#195
Merged
Conversation
Mirrors the album/artist/radio-station action sheet pattern. The long-press on podcast cards and rows now opens an Apple-Music-style sheet with thumbnail, title, author, a Favorite + Play All/Continue + Shuffle quick row, and Refresh + Unsubscribe list rows. Play All / Continue is reactive: when state.currentEpisodeId is set the button reads "Continue" and resumes from the saved playback position via maybeQueueAndPlay; otherwise "Play All" replaces the queue and starts at the top. Shuffle replaces the queue with a shuffled order. Refresh forces a server-side feed re-pull via fetchForPodcast(forceRefresh: true, getUpdates: true) and shows a toast. Adds the koel >= 7.11.0-gated favorite toggle on Podcast + PodcastProvider. The confirmUnsubscribePodcast / unsubscribePodcastWithFeedback helpers move from the deleted podcast_actions_menu.dart into a small podcast_actions.dart so the swipe-to-unsubscribe in the list keeps using them.
|
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:
📝 WalkthroughWalkthroughPodcast gains a mutable ChangesPodcast Favoriting & Action Sheet
Sequence DiagramsequenceDiagram
actor User
participant Card as PodcastCard (UI)
participant Sheet as PodcastActionSheet (UI)
participant Provider as PodcastProvider
participant Playable as PlayableProvider
participant API as API Server
participant Audio as AudioHandler
User->>Card: Long-press podcast
Card->>Sheet: showPodcastActionSheet()
Sheet->>Sheet: Render actions (Favorite, Play/Continue, Shuffle, Refresh, Unsubscribe)
alt User taps Favorite
User->>Sheet: Tap Favorite
Sheet->>Provider: toggleFavorite(podcast)
Provider->>Provider: flip podcast.favorite + notifyListeners
Provider->>API: POST /favorites/toggle {type: "podcast", id}
API-->>Provider: 200 / error
alt error
Provider->>Provider: restore favorite + notifyListeners
end
else User taps Play All
User->>Sheet: Tap Play All
Sheet->>Playable: fetchForPodcast(podcast.id, forceRefresh/getUpdates?)
Playable-->>Sheet: episodes
Sheet->>Audio: replaceQueue(episodes)
else User taps Continue
User->>Sheet: Tap Continue
Sheet->>Playable: fetchForPodcast(podcast.id)
Playable-->>Sheet: episodes
Sheet->>Audio: replaceQueue(episodes, autoPlay: false)
Sheet->>Audio: maybeQueueAndPlay(currentEpisode) (if found)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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 (3)
test/models/podcast_test.dart (1)
35-39: 💤 Low valueLGTM — minor: test title promises "non-bool" but only covers
nulland missing key.An explicit
..['favorite'] = 0(or'false') case would fully justify the title, though the implementation (== true) already handles all non-truevalues identically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/podcast_test.dart` around lines 35 - 39, Update the test 'defaults favorite to false when missing or non-bool' to include an explicit non-boolean value case so the title matches the assertions: after the existing missing/null checks add a variant (e.g., mutate baseJson() with ['favorite'] = 0 or ['favorite'] = 'false') and assert Podcast.fromJson(...).favorite isFalse; this ensures Podcast.fromJson (the deserialization logic) is exercised for non-bool inputs as well.lib/ui/screens/podcasts.dart (1)
151-187: ⚡ Quick win
PodcastRowcan be simplified to aStatelessWidgetAfter removing
_lastTapPosition,_PodcastRowStateno longer holds any mutable fields — it only has abuildmethod. TheStatefulWidget/Statepair is now dead overhead.♻️ Proposed refactor
-class PodcastRow extends StatefulWidget { +class PodcastRow extends StatelessWidget { final Podcast podcast; final AppRouter router; const PodcastRow({Key? key, required this.podcast, required this.router}) : super(key: key); - `@override` - State<PodcastRow> createState() => _PodcastRowState(); -} - -class _PodcastRowState extends State<PodcastRow> { `@override` Widget build(BuildContext context) { - final podcast = widget.podcast; + final podcast = this.podcast; return Card( child: InkWell( onTap: () => router.gotoPodcastDetailsScreen( context, podcastId: podcast.id, ), onLongPress: () => showPodcastActionSheet(context, podcast: podcast), child: ListTile( shape: Border(bottom: Divider.createBorderSide(context)), leading: AlbumArtistThumbnail.sm(entity: podcast, asHero: true), title: Text(podcast.title, overflow: TextOverflow.ellipsis), subtitle: Text( podcast.author, overflow: TextOverflow.ellipsis, style: const TextStyle(color: Colors.white60), ), ), ), ); } }🤖 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 151 - 187, The PodcastRow widget is currently a StatefulWidget with an unused _PodcastRowState; convert PodcastRow into a StatelessWidget by removing the _PodcastRowState class and moving build into PodcastRow (use the constructor params podcast and router as final fields on PodcastRow), updating references from widget.podcast and widget.router to the local fields, and keep all behavior (onTap -> router.gotoPodcastDetailsScreen, onLongPress -> showPodcastActionSheet, AlbumArtistThumbnail.sm, ListTile setup) identical so no logic changes are introduced.test/ui/screens/podcast_action_sheet_test.dart (1)
261-282: ⚡ Quick winMagic
Duration(seconds: 3)couples the Refresh test to an implementation timerAfter
pumpAndSettle()settles animations,pump(const Duration(seconds: 3))advances the clock to drain what looks like a fixed-delay feedback (snackbar/toast). If that timer duration changes in the action sheet implementation, this test silently starts skipping the drain — verifying the mock call count before the async chain completes — which would still pass but for the wrong reason.Consider extracting the duration to a named constant shared between the action sheet and the test, or add an explanatory comment so the coupling is intentional and explicit.
♻️ Proposed improvement (comment + constant reference)
- await tester.pump(const Duration(seconds: 3)); + // Advance past the action sheet's post-refresh feedback timer + // (keep in sync with PodcastActionSheet.kRefreshFeedbackDuration). + await tester.pump(PodcastActionSheet.kRefreshFeedbackDuration);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/screens/podcast_action_sheet_test.dart` around lines 261 - 282, The test currently uses a magic pump(const Duration(seconds: 3)) after pumpAndSettle which couples the test to an internal timer; replace that fragile wait by either using the same named constant the action sheet uses (e.g., ACTION_SHEET_DISMISS_DURATION) or by awaiting the mock interaction via mockito's untilCalled for playableProviderMock.fetchForPodcast(podcast.id, forceRefresh: true, getUpdates: true) after mounting and tapping; remove the hardcoded Duration and ensure the test calls await untilCalled(...) (or imports the shared duration constant) before verifying the fetchForPodcast.called(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/podcast_action_sheet.dart`:
- Around line 126-165: The onTap handlers for Play/Continue/Shuffle and the
Refresh path call _fetchEpisodes() without error handling, so network failures
surface as unhandled exceptions and provide no user feedback; wrap each await
_fetchEpisodes() call in a try/catch, log or report the error via your logger,
show a user-facing error overlay/snackbar (use showOverlay where appropriate),
and return early on failure; ensure the Play/Continue flow still dismisses the
sheet before attempting work (retain Navigator.pop), and keep the existing calls
to audioHandler.replaceQueue, audioHandler.maybeQueueAndPlay, and shuffle
behavior only after a successful fetch.
---
Nitpick comments:
In `@lib/ui/screens/podcasts.dart`:
- Around line 151-187: The PodcastRow widget is currently a StatefulWidget with
an unused _PodcastRowState; convert PodcastRow into a StatelessWidget by
removing the _PodcastRowState class and moving build into PodcastRow (use the
constructor params podcast and router as final fields on PodcastRow), updating
references from widget.podcast and widget.router to the local fields, and keep
all behavior (onTap -> router.gotoPodcastDetailsScreen, onLongPress ->
showPodcastActionSheet, AlbumArtistThumbnail.sm, ListTile setup) identical so no
logic changes are introduced.
In `@test/models/podcast_test.dart`:
- Around line 35-39: Update the test 'defaults favorite to false when missing or
non-bool' to include an explicit non-boolean value case so the title matches the
assertions: after the existing missing/null checks add a variant (e.g., mutate
baseJson() with ['favorite'] = 0 or ['favorite'] = 'false') and assert
Podcast.fromJson(...).favorite isFalse; this ensures Podcast.fromJson (the
deserialization logic) is exercised for non-bool inputs as well.
In `@test/ui/screens/podcast_action_sheet_test.dart`:
- Around line 261-282: The test currently uses a magic pump(const
Duration(seconds: 3)) after pumpAndSettle which couples the test to an internal
timer; replace that fragile wait by either using the same named constant the
action sheet uses (e.g., ACTION_SHEET_DISMISS_DURATION) or by awaiting the mock
interaction via mockito's untilCalled for
playableProviderMock.fetchForPodcast(podcast.id, forceRefresh: true, getUpdates:
true) after mounting and tapping; remove the hardcoded Duration and ensure the
test calls await untilCalled(...) (or imports the shared duration constant)
before verifying the fetchForPodcast.called(1).
🪄 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: ac0a946d-356b-4f8c-8e69-ee8081e5bd96
📒 Files selected for processing (14)
lib/models/podcast.dartlib/providers/podcast_provider.dartlib/ui/screens/podcast_action_sheet.dartlib/ui/screens/podcasts.dartlib/ui/screens/screens.dartlib/ui/widgets/podcast_actions.dartlib/ui/widgets/podcast_card.dartlib/ui/widgets/widgets.darttest/models/podcast_test.darttest/ui/screens/podcast_action_sheet_test.darttest/ui/screens/podcast_action_sheet_test.mocks.darttest/ui/widgets/podcast_card_test.darttest/ui/widgets/podcast_card_test.mocks.darttest/ui/widgets/podcast_row_test.dart
💤 Files with no reviewable changes (2)
- test/ui/widgets/podcast_row_test.dart
- lib/ui/widgets/podcast_actions.dart
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/ui/screens/podcast_action_sheet_test.dart (1)
83-130: ⚡ Quick winConsider adding a
descriptionparameter toPodcast.fake()to avoid full-constructor boilerplate.Both the "capped at 3 lines" and "omits description when empty" tests reconstruct a
Podcastby pulling every field offPodcast.fake()solely to overridedescription. Any future addition of a required field to thePodcastconstructor will break both tests in lockstep.♻️ Suggested simplification (after adding `description` to `Podcast.fake()`)
- final podcast = Podcast.fake(title: 'My Show'); - // description is final on the model; build a podcast with it via - // the named ctor below. - final p = Podcast( - id: podcast.id, - title: podcast.title, - url: podcast.url, - link: podcast.link, - description: description, - author: podcast.author, - imageUrl: podcast.imageUrl, - subscribedAt: podcast.subscribedAt, - lastPlayedAt: podcast.lastPlayedAt, - state: podcast.state, - ); + final p = Podcast.fake(title: 'My Show', description: description);- final base = Podcast.fake(); - final p = Podcast( - id: base.id, - title: base.title, - url: base.url, - link: base.link, - description: '', - author: base.author, - imageUrl: base.imageUrl, - subscribedAt: base.subscribedAt, - lastPlayedAt: base.lastPlayedAt, - state: base.state, - ); + final p = Podcast.fake(description: '');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/screens/podcast_action_sheet_test.dart` around lines 83 - 130, Add an optional description parameter to the Podcast.fake(...) factory so callers can override only the description without rebuilding the whole Podcast via the full constructor; update the Podcast.fake signature to accept String? description (defaulting to the existing fake description), use that value when constructing the fake Podcast, and change the two tests to call Podcast.fake(description: '...') or Podcast.fake(description: '') instead of reconstructing via Podcast(...). Ensure references to Podcast.fake in other tests still compile by keeping the parameter optional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/ui/screens/podcast_action_sheet_test.dart`:
- Around line 329-331: The extra await tester.pump(const Duration(seconds: 3))
after await tester.pumpAndSettle() is ambiguous and brittle; either remove the
stray pump or make the intent explicit by waiting deterministically for the
SnackBar or timed work. Replace the trailing pump with a deterministic strategy:
if you expect a SnackBar after tapping 'Refresh', call tester.pumpAndSettle with
an explicit timeout (e.g., pumpAndSettle(Duration(seconds:5))) or explicitly
dismiss the SnackBar via the ScaffoldMessenger in the test (using the app's
Scaffold/ScaffoldMessenger helper) so the test does not rely on an arbitrary 3s
pump; update the code around the tester.tap('Refresh') / pumpAndSettle calls
accordingly.
---
Nitpick comments:
In `@test/ui/screens/podcast_action_sheet_test.dart`:
- Around line 83-130: Add an optional description parameter to the
Podcast.fake(...) factory so callers can override only the description without
rebuilding the whole Podcast via the full constructor; update the Podcast.fake
signature to accept String? description (defaulting to the existing fake
description), use that value when constructing the fake Podcast, and change the
two tests to call Podcast.fake(description: '...') or Podcast.fake(description:
'') instead of reconstructing via Podcast(...). Ensure references to
Podcast.fake in other tests still compile by keeping the parameter optional.
🪄 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: 933384a5-e010-4697-804d-1aca455eb7a3
📒 Files selected for processing (2)
lib/ui/screens/podcast_action_sheet.darttest/ui/screens/podcast_action_sheet_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/ui/screens/podcast_action_sheet.dart
- Wrap _fetchEpisodes() in try/catch in the Play/Continue, Shuffle and Refresh paths. Refresh shows a 'Refresh failed' overlay; the others silently return because the sheet has already been popped. - Convert PodcastRow to StatelessWidget — _PodcastRowState was empty after the long-press refactor. - Cover non-bool favorite values (0, 1, 'true', 'false') in the Podcast.fromJson test. - Add a test for the Refresh error path.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/ui/screens/podcast_action_sheet_test.dart (1)
329-331:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the fixed 3-second pump.
pumpAndSettle()already drains pending frames/timers; the extrapump(const Duration(seconds: 3))keeps both refresh tests tied to a magic timeout and can flake if the overlay timing changes.Also applies to: 358-360
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/screens/podcast_action_sheet_test.dart` around lines 329 - 331, Remove the hard-coded 3-second pump that follows pumpAndSettle — after the await tester.tap(find.text('Refresh')) and await tester.pumpAndSettle() calls, delete the await tester.pump(const Duration(seconds: 3)); (also remove the identical pump at the other occurrence around lines referencing find.text('Refresh')). Rely on await tester.pumpAndSettle() to drain timers/frames rather than keeping tests tied to a magic timeout so the refresh overlay tests (the taps on find.text('Refresh')) become deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/ui/screens/podcast_action_sheet_test.dart`:
- Around line 329-331: Remove the hard-coded 3-second pump that follows
pumpAndSettle — after the await tester.tap(find.text('Refresh')) and await
tester.pumpAndSettle() calls, delete the await tester.pump(const
Duration(seconds: 3)); (also remove the identical pump at the other occurrence
around lines referencing find.text('Refresh')). Rely on await
tester.pumpAndSettle() to drain timers/frames rather than keeping tests tied to
a magic timeout so the refresh overlay tests (the taps on find.text('Refresh'))
become deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74af6fca-e411-44cb-a97f-008726e23387
📒 Files selected for processing (4)
lib/ui/screens/podcast_action_sheet.dartlib/ui/screens/podcasts.darttest/models/podcast_test.darttest/ui/screens/podcast_action_sheet_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/ui/screens/podcasts.dart
- test/models/podcast_test.dart
Previously, tapping Refresh on the action sheet had two problems: 1. PlayableActionButton's default hideSheetOnTap meant the sheet was popped synchronously while the network call was still in flight. The completion handler then short-circuited on `if (!mounted)`, so neither "Feed refreshed" nor "Refresh failed" ever surfaced — the user got zero feedback even though the call could take seconds. 2. After a successful refresh, the cache was repopulated but the PodcastDetailsScreen's FutureBuilder had no way to know — it kept showing the stale episode list until the user pull-to-refreshed. This commit: - Replaces the Refresh row with an inline ListTile that disables and shows a CupertinoActivityIndicator while in-flight, then dismisses the sheet and shows a 'Feed refreshed' / 'Refresh failed' toast when the call resolves. - Has PlayableProvider.fetchForPodcast call notifyListeners() after a forceful refresh, so any Consumer observing it can rebuild. - Wraps PodcastDetailsScreen's FutureBuilder in Consumer<PlayableProvider> so the notification triggers a rebuild; buildRequest then hits the freshly-repopulated cache and shows the new episodes with no manual pull-to-refresh.
The previous fix attempted to detect "is the sheet still topmost?" via ModalRoute.isCurrent, but that was working around a deeper problem: during a long refresh the user could tap Unsubscribe (which opens a confirm dialog on top), and Refresh's later Navigator.pop(context) would dismiss the *dialog* rather than the sheet — leaving the action sheet stuck with a stale "Refreshing…" spinner while still firing a "Feed refreshed" toast. Simpler model: while a refresh is in flight, every other row in the sheet (Favorite, Play, Shuffle, Unsubscribe) is `enabled: false` — visually dimmed and unresponsive. The user can only wait or swipe the sheet away. _refresh's pop logic is now a plain mounted check; no route bookkeeping needed. Adds a test that taps each row mid-refresh and asserts none of them trigger their delegate or open the confirm dialog.
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 `@test/ui/screens/podcast_action_sheet_test.dart`:
- Around line 59-83: The mount() test harness currently pumps PodcastActionSheet
directly as MaterialApp.home, which fails to exercise modal bottom-sheet route
behavior; update mount() to open the widget via showModalBottomSheet(...,
useRootNavigator: true) inside the MaterialApp (e.g., call showModalBottomSheet
from the mounted app to present PodcastActionSheet) so Navigator.pop() will pop
the sheet route rather than the app route and preserve overlay context for tests
that assert toast/overlay behavior.
🪄 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: e4202d20-f716-4f80-84e5-33378dea0358
📒 Files selected for processing (2)
lib/ui/screens/podcast_action_sheet.darttest/ui/screens/podcast_action_sheet_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/ui/screens/podcast_action_sheet.dart
The previous harness rendered PodcastActionSheet directly as MaterialApp.home, which meant Navigator.pop() in tests popped the *app* route rather than a modal sheet route. That bypassed the modal-to-app boundary that the post-pop toast tests are meant to verify in the first place. Open the sheet through showModalBottomSheet(useRootNavigator: true) — same as production — so the suite actually exercises that boundary.
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
Mirrors the album/artist/radio-station sheet pattern (#190 / #191 / #194) for podcasts. Long-pressing a podcast card or row opens an Apple-Music-style sheet with:
Play All / Continue is reactive on the podcast's playback state: when
podcast.state.currentEpisodeIdis set the button reads Continue and resumes from the saved playback position viaaudioHandler.maybeQueueAndPlay; otherwise it reads Play All and starts at the top of the queue.Refresh calls
PlayableProvider.fetchForPodcast(podcastId, forceRefresh: true, getUpdates: true)which clears the local episode cache and asks the server to re-pull the feed.Adds the optimistic favorite toggle on
Podcast+PodcastProvider(POSTsfavorites/togglewithtype: 'podcast'). TheconfirmUnsubscribePodcast/unsubscribePodcastWithFeedbackhelpers move into a smallpodcast_actions.dartso the swipe-to-unsubscribe in the list keeps using them.Test plan
Summary by CodeRabbit
New Features
Improvements
Tests