Fix episodes queued in Up Next being auto-archived by per-podcast episode limit#5342
Draft
joashrajin wants to merge 4 commits into
Draft
Fix episodes queued in Up Next being auto-archived by per-podcast episode limit#5342joashrajin wants to merge 4 commits into
joashrajin wants to merge 4 commits into
Conversation
…imit The previous filter in checkPodcastForEpisodeLimitBlocking only excluded the currently-playing episode (top of Up Next). Any episode queued deeper in Up Next could be archived when a newer episode arrived and tripped the per-podcast limit during a background refresh, silently removing it from the queue. Extend the filter to skip any episode the user has placed in Up Next, matching the pattern already used in episodeCanBeCleanedUp. Linear: PCDROID-571
Reproduces the PCDROID-571 scenario: episode limit set to 1, two episodes for the same podcast, the older one queued in Up Next behind a newer one. The legacy filter that only excluded `getCurrentEpisode()` would archive the older episode here; with the upNextQueue.contains guard it stays. Linear: PCDROID-571
Collaborator
Generated by 🚫 Danger |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes an auto-archive edge case where episodes queued in Up Next (but not currently playing) could be archived when a per-podcast episode limit is exceeded during background refresh, causing queued episodes to disappear unexpectedly.
Changes:
- Update per-podcast episode-limit auto-archive logic to exclude any episode currently present in the Up Next queue (not just the current episode).
- Add an
androidTestregression test to ensure queued (non-current) Up Next episodes are not archived under the per-podcast limit.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/podcast/EpisodeManagerImpl.kt | Extends the episode-limit exclusion check to cover the full Up Next queue to prevent archiving queued episodes. |
| app/src/androidTest/java/au/com/shiftyjelly/pocketcasts/models/db/AutoArchiveTest.kt | Adds a regression test reproducing the queued-episode auto-archive scenario under a per-podcast limit of 1. |
Comment on lines
+535
to
+536
| runBlocking { upNext.playLast(newerEpisode, onAdd = null) } | ||
| runBlocking { upNext.playLast(olderEpisode, onAdd = null) } |
Contributor
Author
There was a problem hiding this comment.
Good catch, addressed in 549b0c7 — dropped the runBlocking wrappers and call playLast directly from the runTest coroutine.
Contributor
Author
There was a problem hiding this comment.
Update: had to revert this in 2cd7c78 — the instrumented-tests CI job failed after dropping the wrappers. upNext.playLast does withContext(Dispatchers.Default) internally, and the runTest scope here uses an UnconfinedTestDispatcher only as the ioDispatcher — it doesn't control Dispatchers.Default, so the queue can be observed not-yet-populated when the archive check runs immediately after. Matching the existing testAddingInactiveEpisodeToUpNext pattern with runBlocking keeps the ordering reliable.
`runTest` already provides a coroutine scope, so calling the suspend `upNext.playLast` directly is sufficient. Addresses Copilot review feedback on PR #5342.
This reverts commit 549b0c7. The instrumented-tests CI job started failing after that change. `upNext.playLast` dispatches via `withContext(coroutineContext)` where the coroutineContext is `Dispatchers.Default`, which the test's `runTest` scope (using `UnconfinedTestDispatcher` as ioDispatcher) does not control — so the queue can be observed not-yet-populated when the archive check runs immediately after. Restore the `runBlocking` wrappers to match the existing `testAddingInactiveEpisodeToUpNext` pattern and ensure ordering.
Comment on lines
858
to
862
| val episodesToRemove = allEpisodes.drop(episodeLimit) | ||
| .filter { !it.isArchived } | ||
| .filter { (settings.autoArchiveIncludesStarred.value && it.isStarred) || !it.isStarred } | ||
| .filter { playbackManager?.getCurrentEpisode()?.uuid != it.uuid } | ||
| .filter { playbackManager == null || !playbackManager.upNextQueue.contains(it.uuid) } | ||
| if (episodesToRemove.isNotEmpty()) { |
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
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.
Description
checkPodcastForEpisodeLimitBlockingfiltered out only the currently-playing episode (playbackManager.getCurrentEpisode(), which is the top of Up Next). Any episode queued behind the current one in Up Next could still be archived when a newer episode arrived for the same podcast and tripped the per-podcast limit during a background refresh.User-visible effect: when a user has a podcast set to
Auto add to Up Next+ a download/episode limit of 1, an episode queued for later is silently marked as played the next time a new episode arrives via background refresh. The Zendesk reporter perceived this as "the episode I was listening to" being marked played mid-listening.Fix: replace the
getCurrentEpisode().uuid != it.uuidfilter with!playbackManager.upNextQueue.contains(it.uuid). The full-queue check is already the pattern used byepisodeCanBeCleanedUpin the same file (line 886), so this aligns the over-limit path with the existing precedent. The new filter is null-safe to preserve the existing test-onlyplaybackManager = nullbehaviour.Added an
androidTestregression that mirrors the bug: episode limit = 1, two episodes for the same podcast, the older one queued behind a newer one in Up Next. The legacy filter archives the older episode; with this change it stays.Fixes PCDROID-571. User-reported via Zendesk #11253136, with the customer's debug log showing the exact
Auto archiving episode over limit 1 …→Download deleted … Source: UNKNOWNsequence inside a backgroundRefreshPodcastsTaskcycle.Testing Instructions
Auto DownloadandAuto add to Up Next.Or, run the new
androidTest:Checklist
./gradlew spotlessApplyto automatically apply formatting/linting)modules/services/localization/src/main/res/values/strings.xml