feat(chatlist): page the dialog list instead of stopping at fifty - #61
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72768a5695
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Reaching the bottom is the ask. The dialog list is loaded a | ||
| // page at a time — an account's whole history at startup would | ||
| // be a long wait for chats nobody scrolled to — so the cursor | ||
| // arriving near the end is what requests the next one. | ||
| cmds = append(cmds, m.pageAheadCmd()) |
There was a problem hiding this comment.
When the cursor is already within the paging threshold and the user presses Enter, pageAheadCmd() sets pagingMore and returns a command, but the selection branch immediately returns only ChatSelectedMsg, discarding the queued page command. The request therefore never runs while pagingMore remains true, so returning to the chat list leaves pagination permanently disabled for the session; this is reproducible after jumping to the bottom with G and then opening that chat.
Useful? React with 👍 / 👎.
| // Reaching the bottom is the ask. The dialog list is loaded a | ||
| // page at a time — an account's whole history at startup would | ||
| // be a long wait for chats nobody scrolled to — so the cursor | ||
| // arriving near the end is what requests the next one. | ||
| cmds = append(cmds, m.pageAheadCmd()) |
There was a problem hiding this comment.
Pagination is consulted only inside the tea.KeyPressMsg path, but the application's mouse-wheel handler moves this same cursor through chatList.ScrollBy and returns without a command. A user navigating the chat list with the mouse can therefore reach the last loaded dialog but never request the next page, leaving the list capped at the initial 50 until they also press a keyboard key.
Useful? React with 👍 / 👎.
| if raw < pageLimit || after.peer == nil || | ||
| (after.date == cursor.date && after.id == cursor.id) { |
There was a problem hiding this comment.
The pagination cursor consists of date, message ID, and peer, but the stalled-cursor check compares only the first two fields. If the last dialog in the new page belongs to a different peer whose top message has the same ID and timestamp as the previous cursor—a possible collision because channel message IDs are peer-scoped—the page is incorrectly marked exhausted, MoreChatsToLoad becomes false, and all older dialogs are permanently omitted.
Useful? React with 👍 / 👎.
Ceesaxp
left a comment
Ceesaxp
left a comment
There was a problem hiding this comment.
[P1] Pressing Enter near the end permanently wedges pagination. pageAheadCmd runs before m.list.Update and sets pagingMore=true, but when the list reports selection, the early return at lines 888-890 returns only ChatSelectedMsg and discards the queued page command. After returning to the list, shouldPageAhead always rejects further requests because pagingMore never clears. A regression test with 50 rows, cursor 49, and KeyEnter reproduces it. Either request paging only after a movement key, or include the queued commands in the selection return.
|
Fixed in [P1] Enter near the bottom. You offered two fixes; I took neither literally. Threading the command through that return would leave the coupling in place, and restricting paging to movement keys would still let the next early return re-introduce it. The early return is gone instead — the request is made after That is also more correct on its own terms: paging was being decided on where the cursor was before the key moved it. The underlying shape is divergence 39 again, in a new place: [P2] The wheel. Real, and it made the feature keyboard-only. [P2] The stalled cursor. Also real. 21 mutants, no survivors. |
Closes #50. ListChats could paginate to MaxDialogsLimit, but the TUI called LoadChats(50) once at startup and never asked again. The "All" tab showed the fifty most recent dialogs plus whatever arrived later by update; older chats were reachable only through search. The cursor asking for more is what asks now: reaching within ten rows of the end requests the next page, ahead of the bottom rather than at it, so the rows arrive before the reader runs out of them. One request at a time — holding j at the bottom would otherwise be a round trip per keystroke — and none at all once a short page has said the list is exhausted, or while a filter is applied, since filtering narrows what is already loaded and the end of three matches says nothing about the dialog list. The cursor stays on the client. It is a gotd InputPeer, and a chat list holding one would be a UI component carrying a protocol type it can do nothing else with; LoadMoreChats and MoreChatsToLoad are the whole surface. UnreadCountMsg is deleted rather than fixed. The issue asks that a page sum stop being announced as the account total — but the sum has been announced to nobody since the status bar was retired in 59960be, which was its only consumer. What is on screen comes from ChatStore.TotalUnread over loaded chats, which is honest about its scope and now grows as pages arrive. Fourth dead message type found in this repo. Two seams, both following notifySettingsGetter's precedent in the same package: pageDialogs takes the page fetcher as a function, so every rule that ends or advances pagination is testable without a server, and the chat list holds a two-method dialogSource rather than the concrete client, so whether a keystroke asks — and asks once — is testable with a fake. The mutation pass found two tests passing for the wrong reason. Both loop guards were covered by tests whose fakes let the SHORT-PAGE rule fire first, so neither the stalled cursor nor the nil continuation peer was ever exercised. Full pages and an endless fake fixed both; the short-page test now counts requests, because "it stopped" is not the same claim as "it stopped because of this". 18 mutants, no survivors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three review findings, one of them a real wedge. [P1] pageAheadCmd does two things at once: it marks a request in flight and returns the command that clears the mark. It ran BEFORE m.list.Update, and the selection branch then returned ChatSelectedMsg on its own — so pressing Enter near the bottom of the list charged for a request whose command was discarded, and pagingMore stayed true for the rest of the session. G then Enter was enough. Reproduced before fixing. The fix is not to thread the command through that return but to remove the early return: the request is made after the move, once, and every path falls through to the single tea.Batch at the end. Which is also more correct — paging was being decided on where the cursor was before the key moved it. This is divergence 39's lesson again, in a new place. A call that mutates state AND returns the thing that undoes it is a call every site has to remember to propagate, and one of them forgets. [P2] The wheel moves the same cursor the keyboard does and asked for nothing, so a mouse user reached the last loaded dialog and stopped there. ScrollBy returns a command now, and the app's wheel handler returns it. [P2] The stalled-cursor check compared date and id but not the peer. A message id is scoped to its peer, so two dialogs in different channels can share an id and a date — and the loop would then call the cursor stalled when it had in fact moved, dropping every older dialog for the session. sameCursor compares the peer's IDENTITY rather than the InputPeer value: those carry access hashes the server is free to reissue, and two values describing one peer would otherwise compare unequal. 21 mutants, no survivors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c205dcc to
f0ce984
Compare
1 participant
Closes #50.
What
ListChatscould paginate toMaxDialogsLimit, but the TUI calledLoadChats(50)once at startup and never asked again. The "All" tab showed the fifty most recent dialogs plus whatever arrived later by update; older chats were reachable only through search.The cursor is what asks now. Coming within ten rows of the end requests the next page — ahead of the bottom rather than at it, so rows arrive before the reader runs out of them.
It declines to ask when asking would be pointless: while a request is in flight (holding
jat the bottom would otherwise be a round trip per keystroke), once a short page has said the list is exhausted, and while a filter is applied — filtering narrows what is already loaded, so the end of three matches says nothing about the dialog list.The cursor stays on the client. It's a gotd
InputPeer; a chat list holding one would be a UI component carrying a protocol type it can do nothing else with.LoadMoreChatsandMoreChatsToLoadare the whole surface.UnreadCountMsgis deleted, not fixedThe issue asks that a page sum stop being announced as the account total. It turns out the sum has been announced to nobody since
59960beretired the status bar, which was its only consumer — the fourth dead message type I've found in this repo.What is actually on screen comes from
ChatStore.TotalUnreadover loaded chats. That is honest about its scope, satisfies the issue's stated minimum, and now grows as pages arrive.Two seams, both with precedent
pageDialogstakes the page fetcher as a function, so every rule that ends or advances pagination is testable without a server. The chat list holds a two-methoddialogSourcerather than the concrete client, so whether a keystroke asks — and asks once — is testable with a fake. Both follownotifySettingsGetterininternal/telegram.What the mutation pass caught
Two tests were passing for the wrong reason.
TestACursorTheServerDidNotAdvanceEndsTheListandTestAPageWithNothingToContinueFromEndsTheListboth used fakes whose pages were shorter than requested — so the short-page rule ended the list before the guard each test names ever ran. Both would have passed against a build with their guard deleted.Fixed with full pages, and the short-page test now counts requests: "it stopped" is not the same claim as "it stopped because of this".
Acceptance criteria
Testing
Build, vet,
go test ./...,gofmtclean. 18 mutants, no survivors.🤖 Generated with Claude Code