fix: correct episode pagination for sorts other than newest-first - #972
Open
xoiram wants to merge 1 commit into
Open
fix: correct episode pagination for sorts other than newest-first#972xoiram wants to merge 1 commit into
xoiram wants to merge 1 commit into
Conversation
return_podcast_episodes_capitalized capped each UNION branch (Episodes, YouTubeVideos) with `ORDER BY pubdate DESC LIMIT (limit+offset)` regardless of the requested sort, then re-sorted the truncated set in the outer query. That's only valid when sorting newest-first: for any other sort (oldest first, title, duration) on a podcast with more episodes than the current page window, the per-branch cap discarded rows the outer sort actually needed. In practice this made "oldest first" pagination loop over the same newest-N episodes forever instead of ever reaching older ones, and made completed/incomplete/in-progress filters silently drop matches that weren't among the newest N raw rows (even though the separate, unrestricted count query correctly reported them). Fixes, for both the Postgres and MySQL code paths: - Each per-branch subquery now orders by the actually-requested sort column/direction instead of a hardcoded recency order, restoring the top-K-merge correctness of the per-branch LIMIT for every sort. - The completed/incomplete/in-progress filter is now pushed into each branch's WHERE before its LIMIT, instead of only being applied after truncation. - Added an explicit tiebreaker (episodeid/videoid per branch, is_youtube + Episodeid at the outer level) so ties in the sort column can't produce a different row order across separate page requests as per_branch_limit grows, which could otherwise reintroduce the same duplicate/skip behavior via a different path. - Consolidated the sort-column and filter-clause mappings, which had drifted into 4 and 6 duplicated match blocks respectively across the two DB backends — the kind of divergence that caused the original bug. Adds in-binary regression tests (rust-api has no lib target, so these can't live under tests/) covering: oldest-first pagination visiting every episode without duplicates, a completed-filter match outside the newest page, and pagination stability when the sort column is fully tied. Verified against a real Postgres, including confirming each test fails against the pre-fix query and passes after. Wires a new rust-backend-tests CI job (its own ephemeral Postgres service) to run these, since the existing backend-tests job's Python suite is already broken (imports a FastAPI app module that no longer exists) and untested. Also gitignores rust-api/target/, which wasn't previously excluded.
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.
I've recently migrated from Pocket Casts, and this project seems to tick all my boxes for what I want. However, when trying to navigate some of my bigger podcast backlogs I noticed "oldest first" doesn't really work in the web UI. This fixes that. It also adds a small test to verify the behavior against the postgres docker that was present.
I've also published a docker image with these changes and verified it works on my installation. Docker image is here.
Full disclosure: This was done with the help of Claude Sonnet 5.
Claude summary of the changes:
return_podcast_episodes_capitalized capped each UNION branch (Episodes, YouTubeVideos) with
ORDER BY pubdate DESC LIMIT (limit+offset)regardless of the requested sort, then re-sorted the truncated set in the outer query. That's only valid when sorting newest-first: for any other sort (oldest first, title, duration) on a podcast with more episodes than the current page window, the per-branch cap discarded rows the outer sort actually needed. In practice this made "oldest first" pagination loop over the same newest-N episodes forever instead of ever reaching older ones, and made completed/incomplete/in-progress filters silently drop matches that weren't among the newest N raw rows (even though the separate, unrestricted count query correctly reported them).Fixes, for both the Postgres and MySQL code paths:
Adds in-binary regression tests (rust-api has no lib target, so these can't live under tests/) covering: oldest-first pagination visiting every episode without duplicates, a completed-filter match outside the newest page, and pagination stability when the sort column is fully tied. Verified against a real Postgres, including confirming each test fails against the pre-fix query and passes after.
Wires a new rust-backend-tests CI job (its own ephemeral Postgres service) to run these, since the existing backend-tests job's Python suite is already broken (imports a FastAPI app module that no longer exists) and untested. Also gitignores rust-api/target/, which wasn't previously excluded.