fix(macos): advance sidebar pagination by DB offset, not accumulated count (address Codex review on #31924)#32103
Conversation
…count After the cold-start drain loop, ConversationRestorer set serverOffset to foreground.conversations.count. The conversations endpoint appends injected pinned rows on page 1 (offset 0), so the accumulated count overshoots the server's DB cursor and the next loadMoreConversations() skips that many rows. Carry the server-provided nextOffset (DB pagination cursor) through the synthesized fetchAllConversationPages response and use it for serverOffset, mirroring the appendConversations pagination path. Addresses Codex review on #31924.
![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: 8755666912
ℹ️ 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".
| // server-provided nextOffset so incremental "load more" resumes at | ||
| // the right cursor instead of skipping rows. Fall back to advancing | ||
| // by one full page when the daemon omits nextOffset. | ||
| nextOffset = response.nextOffset ?? (offset + pageSize) |
There was a problem hiding this comment.
When response.nextOffset is absent, this fallback advances by a full page (offset + pageSize) instead of by the rows actually returned. On older daemons that omit nextOffset, the terminal page is often partial, so nextOffset becomes too large (e.g., 450 rows drained with page size 200 yields 600). That inflated value is then assigned to serverOffset and the next pagination request can start past unseen rows, skipping conversations. The fallback should use offset + response.conversations.count (or equivalent accumulated count) to represent the true DB cursor reached.
Useful? React with 👍 / 👎.
1 participant
Summary
After the cold-start drain loop,
ConversationRestorersetserverOffset = foreground.conversations.count. But the conversations endpoint appends injected pinned rows on page 1 (offset 0, server-side), so the accumulated count overshoots the server's DB cursor — the next incrementalloadMoreConversations()(which callsfetchConversationList(offset: serverOffset, ...)) then skips that many conversations.This carries the server-provided
nextOffset(DB pagination cursor) through the synthesizedfetchAllConversationPagesresponse and usesforeground.nextOffset ?? foreground.conversations.countforserverOffset, mirroring the already-fixedappendConversationspath inConversationListStoreso behavior is consistent.Notes
offset + pageSize==pagesFetched * pageSize) only engages for daemons that omitnextOffset; it is exercised only whenhasMoreis false (pagination disabled), so it introduces no regression.fetchConversationListPaginatesUntilHasMoreIsFalsestill assertsserverOffset == 450(mock has no pinned-row injection, so count == nextOffset).Addresses Codex review on #31924.