Skip to content

fix(macos): advance sidebar pagination by DB offset, not accumulated count (address Codex review on #31924) #32103

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
siddseethepalli merged 1 commit into from
May 26, 2026
Merged

fix(macos): advance sidebar pagination by DB offset, not accumulated count (address Codex review on #31924) #32103

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions clients/macos/vellum-assistant/Features/MainWindow/ConversationRestorer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,7 @@ final class ConversationRestorer {
}

// serverOffset is set by fetchConversationList before merging foreground +
// background, so it reflects foreground-only count for correct pagination.
// background, so it reflects the foreground-only DB cursor for correct pagination.
log.info("Restored \(restoredConversations.count) conversations from daemon (hasMore: \(response.hasMore ?? false))")
delegate.restoreLastActiveConversation()
}
Expand Down Expand Up @@ -692,10 +692,12 @@ final class ConversationRestorer {
let uniqueBackground = (background?.conversations ?? []).filter {
seenIds.insert($0.id).inserted
}
// Set serverOffset from foreground count BEFORE merging.
// loadMoreConversations pages the foreground endpoint only,
// so the offset must not include merged background rows.
self.delegate?.serverOffset = foreground.conversations.count
// Set serverOffset to the foreground DB cursor BEFORE
// merging. loadMoreConversations pages the foreground
// endpoint only, so the offset must not include merged
// background rows; nextOffset is the DB cursor (see
// fetchAllConversationPages), not the inflated row count.
self.delegate?.serverOffset = foreground.nextOffset ?? foreground.conversations.count
let merged = ConversationListResponse(
type: foreground.type,
conversations: foreground.conversations + uniqueBackground,
Expand Down Expand Up @@ -735,6 +737,7 @@ final class ConversationRestorer {
let pageSize = Self.conversationListPageSize
var accumulated: [ConversationListResponseItem] = []
var firstPage: ConversationListResponse?
var nextOffset = 0

for page in 0..<Self.conversationListMaxPages {
let offset = page * pageSize
Expand All @@ -745,19 +748,27 @@ final class ConversationRestorer {
}
if firstPage == nil { firstPage = response }
accumulated.append(contentsOf: response.conversations)
// Track the DB cursor the server actually reached. On page 1
// (offset 0) the endpoint appends injected pinned rows, so the
// accumulated count overshoots the DB offset; prefer the
// 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)
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Derive fallback cursor from returned row count

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 👍 / 👎.

let hasMore = response.hasMore ?? false
if !hasMore || response.conversations.isEmpty { break }
}

guard let first = firstPage else { return nil }
// Groups are sent with the first page only; preserve them on the
// synthesized response. `hasMore` is forced to false because we've
// already exhausted the server (or hit the safety cap).
// already exhausted the server (or hit the safety cap). `nextOffset`
// carries the DB cursor reached (tracked above).
return ConversationListResponse(
type: first.type,
conversations: accumulated,
hasMore: false,
nextOffset: nil,
nextOffset: nextOffset,
groups: first.groups
)
}
Expand Down