Skip to content

fix: pagination timeout no longer clears isLoadingMoreMessages #9327

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
ashleeradka merged 1 commit into from
Feb 26, 2026
Merged

fix: pagination timeout no longer clears isLoadingMoreMessages #9327

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: 19 additions & 6 deletions clients/shared/Features/Chat/ChatViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,9 @@ public final class ChatViewModel: ObservableObject {
/// True while a previous-page load is in progress (brief async delay for UX).
@Published public var isLoadingMoreMessages: Bool = false

/// Timeout task that resets `isLoadingMoreMessages` if the daemon never responds.
/// Timeout task that logs a warning if the daemon takes too long to respond
/// to a pagination request. The flag is intentionally NOT cleared here —
/// see the comment in `loadPreviousMessagePage()` for rationale.
private var loadMoreTimeoutTask: Task<Void, Never>?

/// The subset of messages that are actually displayed (excludes subagent notifications
Expand Down Expand Up @@ -460,13 +462,20 @@ public final class ChatViewModel: ObservableObject {
// All local messages are visible — fetch the next page from the daemon.
guard hasMoreHistory, let cursor = historyCursor, let sessionId else { return false }
isLoadingMoreMessages = true
// Safety timeout: if the daemon drops the connection or never responds,
// reset the flag so the user can retry.
// Safety timeout: log a warning if the daemon is slow, but do NOT
// clear isLoadingMoreMessages here. Callers (ThreadSessionRestorer,
// IOSThreadStore) use `vm.isLoadingMoreMessages` to decide whether
// a history response is a pagination load. If the timeout clears the
// flag before the response arrives, the late-but-valid response is
// misclassified as an initial load and replaces all messages instead
// of prepending. The flag is properly cleared by populateFromHistory
// when the response arrives, or by reconnect/thread-switch logic if
// the daemon disconnects.
loadMoreTimeoutTask?.cancel()
loadMoreTimeoutTask = Task { @MainActor [weak self] in
try? await Task.sleep(nanoseconds: 15_000_000_000) // 15 seconds
guard let self, self.isLoadingMoreMessages else { return }
self.isLoadingMoreMessages = false
try? await Task.sleep(nanoseconds: 30_000_000_000) // 30 seconds
guard let self, !Task.isCancelled, self.isLoadingMoreMessages else { return }
log.warning("Pagination request still pending after 30s — daemon may be unresponsive")
Comment on lines 475 to +478
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot Feb 26, 2026

Choose a reason for hiding this comment

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

🔴 Removing timeout reset of isLoadingMoreMessages causes permanent spinner when daemon silently drops a pagination request

If the daemon silently drops or never responds to a pagination history_request (without fully disconnecting), isLoadingMoreMessages will remain true forever. The guard at ChatViewModel.swift:444 (guard hasMoreMessages, !isLoadingMoreMessages else { return false }) then blocks all future pagination attempts, leaving the user stuck with a permanent loading spinner and no way to retry.

Root Cause and Impact

The old code had a 15-second safety timeout that would reset isLoadingMoreMessages = false, allowing the user to retry. This PR removes that reset and only logs a warning (ChatViewModel.swift:478). The comment at line 472 claims the flag is "properly cleared by … reconnect/thread-switch logic if the daemon disconnects," but this is not accurate:

  1. Reconnect handler doesn't clear it: The daemonDidReconnect observer (ChatViewModel.swift:641-711) never touches isLoadingMoreMessages.
  2. resetMessagePagination() is dead code: The method at ChatViewModel.swift:486 does clear the flag, but it is never called from anywhere in the codebase — no caller exists in shared, macOS, or iOS code.
  3. Thread-switch doesn't clear it: ThreadManager.trimPreviousThreadIfNeeded at ThreadManager.swift:648 actually skips trimming when isLoadingMoreMessages is true, but doesn't reset it.

The only live paths that clear isLoadingMoreMessages are: (a) populateFromHistory when isPaginationLoad is true (ChatViewModel.swift:2107), and (b) error paths in ThreadSessionRestorer.requestPaginatedHistory / IOSThreadStore.requestPaginatedHistory when the send fails synchronously. None of these help when the request is sent successfully but the daemon silently drops it.

Impact: The user sees an infinite spinner at the top of the message list and cannot load older messages. The only recovery is switching threads or restarting the app.

Prompt for agents 
In clients/shared/Features/Chat/ChatViewModel.swift, the timeout task at lines 475-478 needs to be restored as a safety net that clears isLoadingMoreMessages after the timeout period. The current code only logs a warning. While the original 15-second timeout caused the misclassification bug described in the PR, the solution should not remove the safety reset entirely. Instead, consider a longer timeout (e.g., 30 seconds as currently set) that does clear isLoadingMoreMessages, but also sets a separate flag (e.g., paginationTimedOut) that the handleHistoryResponse callers in ThreadSessionRestorer.swift:241 and IOSThreadStore.swift:430 can check. This way, if a late response arrives after timeout, the callers can detect it was a pagination response even though isLoadingMoreMessages was cleared by timeout.

Alternatively, the simpler fix is to ensure resetMessagePagination() is actually called from the reconnect handler at line 672 (inside the daemonDidReconnect observer) and from thread-switch logic, since the PR comment claims these paths clear the flag but they currently do not. At minimum, add self?.isLoadingMoreMessages = false in the reconnect observer block around line 659, alongside the other state resets.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

}
Comment on lines +476 to 479
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

P1 Badge Clear pagination lock when timeout fires

If the daemon never returns a history_response for a pagination request while the connection stays up (for example, the request is dropped server-side), this timeout path now only logs and never resets isLoadingMoreMessages. Because loadPreviousMessagePage() refuses to start when that flag is true, the thread can get stuck in a permanent "loading more" state and users cannot retry pagination until an unrelated reset happens.

Useful? React with 👍 / 👎.

onLoadMoreHistory?(sessionId, cursor)
// The loading indicator is cleared by populateFromHistory when the response arrives.
Expand All @@ -478,6 +487,9 @@ public final class ChatViewModel: ObservableObject {
displayedMessageCount = Self.messagePageSize
historyCursor = nil
hasMoreHistory = false
loadMoreTimeoutTask?.cancel()
loadMoreTimeoutTask = nil
isLoadingMoreMessages = false
}

// MARK: - On-Demand Content Rehydration
Expand Down Expand Up @@ -2161,6 +2173,7 @@ public final class ChatViewModel: ObservableObject {
messageLoopTask?.cancel()
streamingFlushTask?.cancel()
cancelTimeoutTask?.cancel()
loadMoreTimeoutTask?.cancel()
// refinementFailureDismissTask and refinementFlushTask are accessed via
// @MainActor computed properties (forwarded from ChatMessageManager), which
// cannot be referenced from nonisolated deinit. Both tasks use [weak self],
Expand Down
Loading