fix(chat): use paginatedVisibleMessages for empty/skeleton routing to fix blank chat on load#24095
Conversation
… ChatView The ChatView routing logic (isEmptyState, shouldShowSkeleton) previously checked viewModel.messages.isEmpty, but the actual rendering used viewModel.paginatedVisibleMessages. These properties come from different @observable objects (ChatMessageManager vs ChatPaginationState) connected via Combine. When messages was non-empty but paginatedVisibleMessages was empty — due to the _modify accessor's deferred Combine publish or all messages being filtered by ChatVisibleMessageFilter — the view fell through to activeConversationContent rendering nothing: the blank chat area shown in the bug report. Fix: use paginatedVisibleMessages.isEmpty for both routing checks so the decision is consistent with what is actually displayed. Also removes the unnecessary Combine re-subscription with dropFirst() in resetMessagePagination(). The ChatPaginationState belongs to a specific ChatViewModel (per-conversation), so the existing subscription already tracks the correct messageManager. The re-subscription added fragility without benefit since the method already calls recomputeVisibleMessages synchronously. Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
| private var isEmptyState: Bool { | ||
| viewModel.messages.isEmpty && viewModel.isHistoryLoaded | ||
| viewModel.paginatedVisibleMessages.isEmpty && viewModel.isHistoryLoaded | ||
| } | ||
|
|
||
| private var shouldShowSkeleton: Bool { | ||
| viewModel.messages.isEmpty && !viewModel.isHistoryLoaded | ||
| viewModel.paginatedVisibleMessages.isEmpty && !viewModel.isHistoryLoaded | ||
| } |
There was a problem hiding this comment.
🚩 Subtle behavior change: empty state now shown when messages exist but are all hidden
The switch from viewModel.messages.isEmpty to viewModel.paginatedVisibleMessages.isEmpty at clients/macos/vellum-assistant/Features/Chat/ChatView.swift:106 and :110 introduces a semantic change. If a conversation has messages but they are all hidden (subagent notifications, hidden messages, or messages without renderable content filtered by ChatVisibleMessageFilter), paginatedVisibleMessages will be empty while messages would not be.
Previously, this would render activeConversationContent (empty message list + composer). Now it renders the empty state view (greeting + built-in composer). Both paths provide a working composer, so the user can still interact. This is likely an intentional UX improvement — showing the empty state greeting when nothing is visible is arguably better than an empty message list. However, the reviewer should confirm this edge case (all-hidden messages) was considered.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is an intentional improvement, already documented in the PR description under the "All-filtered edge case" checklist item. When all messages are filtered (hidden/subagent/phantom), showing the empty state greeting with a working composer is better UX than showing a blank message list with a composer — the user gets context that the conversation is empty from their perspective rather than staring at a blank screen.
Summary
Fixes a bug where loading the app sometimes shows a blank chat area (no messages, no skeleton, no greeting) despite a conversation being selected in the sidebar.
ChatView's display routing (isEmptyState,shouldShowSkeleton) checkedviewModel.messages.isEmpty, but the actual rendering usedviewModel.paginatedVisibleMessages— these come from different@Observableobjects (ChatMessageManagervsChatPaginationState) connected via Combine, and can desync when the_modifyaccessor defers its Combine publish or when all messages are filtered byChatVisibleMessageFilter. The fix usespaginatedVisibleMessages.isEmptyfor both routing checks so branching is consistent with what's actually displayed.Also removes the unnecessary Combine re-subscription with
dropFirst()inresetMessagePagination()— theChatPaginationStateis per-ChatViewModelso the existing subscription frominitalready tracks the correctmessageManager, and the method already callsrecomputeVisibleMessagessynchronously.Review & Testing Checklist for Human
Notes
messagesandpaginatedVisibleMessagesregardless of specific trigger.resetMessagePagination()is never called externally in the current codebase, so removing its re-subscription is low-risk. The original subscription frominitremains active.ChatContentViewuses a different routing pattern and is not affected.Link to Devin session: https://app.devin.ai/sessions/ca7df320069f4c0da87d33cfa3e401ab
Requested by: @Jasonnnz