Repository navigation
fix(ui): restart pagination when the feed or category page falls past the end - #4519
Conversation
jamalkamaladdin
left a comment
jamalkamaladdin
left a comment
There was a problem hiding this comment.
Read this against showUnreadPage and the query builder. Two things I checked
-
WithOffsetininternal/storage/entry_query_builder.goonly assigns whenoffset > 0, so reusing an already built query and calling.WithOffset(0)on it would silently keep the stale offset. The closure avoids that, and the note in your description is accurate. -
Both handlers pass the reassigned
offsetintogetPagination, so the restarted page also gets correct Previous and Next links instead of links built from the old offset.
One decision worth surfacing for the maintainer: showUnreadPage still carries this guard inline with the full builder chain written out twice. After this PR the same rule lives in three files in two different shapes. Moving unread_entries.go to the closure form here would leave one pattern to read, at the cost of touching a file the bug report does not require.
… the end Marking a page as read shrinks the unread list under the current offset. On the last page that leaves the offset past the end, and the template hides pagination whenever there is nothing to render, so the view shows "no unread entries" with no way back even though the header still reports the remaining count. Reloading or reopening the URL keeps landing on the same dead end. Fall back to the first page when the offset is past the end, which is what showUnreadPage already does for the global unread view. A page that is still within range is untouched, and a genuinely empty feed keeps its alert.
7a6c440 to
2a8e8be
Compare
|
Thanks for reading it that closely, both of your checks match what I found. On the third point I left Worth being honest about what it buys, too: the three handlers still would not read alike afterwards. I did trim the comment in One thing your comment made me go back and check, since the old comment blamed the template. |
Fixes #4386
Context
On the last page of a feed, "Mark this page as read" leaves an empty page with no navigation. The unread list shrinks under the current offset, so the offset is now past the end; the template only renders the pagination block when there are entries, so the view falls back to the "no unread entries" alert with no Previous link, even though the header still reports the entries that remain. Reloading, or reopening the same URL later, lands on the same dead end.
The global unread view already handles this:
showUnreadPageresets the offset to 0 and requeries whenoffset >= countUnread && countUnread > 0. The feed and category views are the other two with a "Mark this page as read" button and never got the same guard.Changes
Apply that guard to
showFeedEntriesPageandshowCategoryEntriesPage. The query moves into a small closure so the fallback does not duplicate the builder chain —WithOffsetignores 0, so the same builder cannot be reused directly.Scope
The three views that carry the "Mark this page as read" button are the feed, category and unread lists, and
markPageAsReadActioninapp.jsonly reloads in place whendata-show-only-unreadis set, which only the feed and category templates set. So those two were the only ones missing the guard, and no other list view can reach this dead end through that button.Other list views can strand on a stale offset by different routes,
/starredwhen an entry is unstarred,/historywhen one is marked unread,/search?unread=1when a result is auto-marked read on view. They are left alone here since they have no bulk action and are not what #4386 reports.I also considered fixing this in the template instead, since
getPaginationalready returnsShowPrevandPrevOffsetcorrectly at a stale offset and the view discards them under{{ if not .entries }}. Rendering pagination for an empty page would restore the Previous link, but it would still show the "no unread entries" alert while unread entries exist just below the offset, so the reader is told the wrong thing. Resetting the offset shows them instead, and matchesshowUnreadPage.Resetting to the first page rather than clamping to the new last page is also for parity with
showUnreadPage.User impact
Marking the last page as read lands on the first page of what is left instead of an empty page. A page still within range is unaffected, and a feed with genuinely nothing unread keeps its alert.
Verification
Built and run against a local PostgreSQL instance with a feed of 25 unread entries and 20 entries per page.
offset=20, 25 unreadoffset=20after marking that page read (20 unread)offset=100, 25 unreadoffset=0/offset=20, 0 unread/category/1/entries?offset=20behaves identically before and after the change.go vet ./internal/ui/...,gofmtandgolangci-lint run ./internal/ui/...are clean.go test ./...passes exceptTestParseAdminPasswordFileOptionWithEmptyFile, which fails on this Windows host before and after the change because it writes its temporary file toC:\WINDOWS.