refactor(messages): page the feed by opaque cursor instead of offset (#22) - #96
Merged
Merged
Conversation
GET /api/messages declares limit/onlyMine/tag/cursor and no offset at all, so the feed's limit/offset paging was sending a parameter the server ignores: page 2 re-served the head and drifted whenever a new post arrived between pages. Switch the feed to the documented cursor/nextCursor contract, mirroring the shape :feature:directmessages already uses for /api/dm/conversations: - MessagesApi.getMessages takes an optional `cursor` instead of `offset`; Retrofit omits it for the first page. - PaginationDto carries `nextCursor`, surfaced as MessagesResponse.nextCursor (blank counts as absent). The envelope now also accepts rows under the documented `messages` key as well as `data`, read through `rows`. - MessagesRepository.refreshFeed() returns the next cursor and loadMoreFeed() takes one, replacing the boolean hasMore / currentCount offset pair. The cursor is opaque: it is stored and handed back verbatim, never parsed, constructed or compared. - The feed ViewModel holds the cursor in its existing paging state; a refresh-from-top clears it, and a null cursor ends the feed. Room stays the source of truth: a refresh still replaces the head, a page append still writes to the tail, and rows keyed by id cannot duplicate. Message search is left on its current paging: the spec's /api/messages/search declares only q/limit/onlyMine, with no cursor to pass back. Tests: a new MessagesFeedCursorPagingTest pins that page 1 sends no cursor and no offset, that page 2 echoes the cursor verbatim, that a post arriving at the head between pages neither duplicates nor skips rows, that a null nextCursor ends the feed, and that a refresh resets the cursor; MessagesResponseTest pins the envelope/cursor parsing; the feed ViewModel tests cover cursor threading and the reset on refresh. Closes #22
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.
Closes #22. Part of epic #17.
Why this matters more than it looks
The live spec declares exactly four query parameters on
GET /api/messages:limit,onlyMine,tag,cursor. There is nooffset. So the app'slimit/offsetpaging was sending aparameter the server does not document — and a confirmed live call returns
pagination: { total, offset, limit, hasMore, nextCursor }withnextCursoras an opaque base64keyset token. This was drift waiting to happen on exactly the workload a social feed hits
constantly: new posts arriving at the head between page fetches.
What changed
Mirrors the
:feature:directmessagesshape, so the two modules now page identically — repositorytakes a cursor and returns the next one, the caller holds it,
nullmeans end of list.MessagesApi.getMessagestakes@Query("cursor") cursor: String? = nullinstead ofoffset.Retrofit omits it entirely for page 1, so
offsetnever appears on the wire again.PaginationDtogainednextCursor;MessagesResponseexposes it (blank treated as absent) anda
rowsaccessor reading from eitherdataormessages, matchingConversationsResponse.rows.refreshFeed(): ApiResult<String?>/loadMoreFeed(cursor): ApiResult<String?>replace theBoolean hasMore/currentCountoffset pair. The cursor is only ever stored and handed back— never parsed, sliced, built or compared.
MessagesFeedViewModelholds it in the existingFeedTransientState;canLoadMorederives fromnextCursor != null;refresh()nulls it so paging restarts from the top.maxFeedOrder + 1,rows keyed by id so a repeat updates in place.
Search is deliberately left alone:
GET /api/messages/searchdeclares onlyq,limitandonlyMine— there is no cursor to pass back.Verification
./gradlew :app:assembleDebug testDebugUnitTest→ BUILD SUCCESSFUL, 727 tests, 0 failures.The regression the issue is really about is covered by a MockWebServer
Dispatcherthat serves theoffset-drifted page (duplicate
m3, skippedm1) if anoffsetquery ever appears — so aregression to offset paging is caught by the row assertions and by an explicit "no
offsetparameter" assertion. Mutation-checked: dropping the cursor from
loadMoreFeedmakes botha post arriving at the head between pages neither duplicates nor skips rowsandpage two hands the cursor back verbatim and sends no offsetfail, and both pass again on restore.Reviewer note — a pre-existing bug found but NOT fixed here
DefaultMessagesRepository.createMessageplaces a new post atmaxFeedOrder() - 1. The feed sortsby
feedOrder ASC, so with more than two cached rows a freshly created message lands mid-feedrather than at the top. It needs a
minFeedOrder()DAO query. The existing test only covers asingle-row feed, which is why it passes today. Left out of scope because it sits in composer code
that #19/#20 will touch — filed separately so it does not get lost.
Also unrelated and untouched:
GET /api/messages/{id}/repliesreturns rows underreplies, whichMessagesResponse.rowsdoes not read.