Optimize conversation restoration startup and reduce redundant sidebar recomputation#23317
Merged
Merged
Conversation
…ess animations, cache derived properties - Eliminate double activation in ConversationRestorer.handleConversationListResponse(): Compute the single activation target (saved last-active or first visible) up front and activate exactly once, instead of activating firstVisible then potentially re-activating via restoreLastActiveConversation(). - Refactor activeConversationId didSet into explicit performActivation/performDeactivation: The didSet now performs only lightweight bookkeeping (UserDefaults, anchor clearing). Heavy side effects (VM creation, daemon notification, observation setup) moved to performActivation(for:) and performDeactivation() methods that callers invoke explicitly. - Suppress animations during bulk conversation list restoration: Wrap the bulk conversations array assignment in withTransaction with disablesAnimations to skip animation interpolation for ~50 rows on startup. - Cache computed sidebar properties as stored properties in ConversationListStore: Convert groupedConversations, visibleConversations, sortedGroups, unseenVisibleConversationCount, and archivedConversations from computed to stored properties recomputed once per conversations/groups mutation via didSet. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
Contributor
Author
🤖 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:
|
Protocol requirement added in ConversationRestorer.swift for the single-activation-target optimization. The mock needs it to compile. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
The visibleAfter/visibleBefore filters only checked !isArchived, while visibleConversations also excludes .private conversations. This could cause archiveConversation to activate a private conversation as the next selection target. Now consistent with visibleConversations. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
ashleeradka
previously approved these changes
Apr 3, 2026
…tion, guard empty recompute - Make activeConversationId private(set) on ConversationSelectionStore. Remove the public setter from ConversationManager facade — only performActivation(for:) and performDeactivation() can change selection. Prevents future code from silently bypassing VM creation, daemon notification, and observer wiring. - Add performDeactivation() fallback in closeConversation when both branches fail to find a next conversation, preventing stale activeConversationId. - Guard recomputeDerivedProperties() to skip expensive sort/filter/bucket work when conversations is empty (e.g. when groups is assigned before conversations during restoration). Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
…es calls Per-field struct mutations on conversations[index] each trigger conversations.didSet → recomputeDerivedProperties() (O(N log N) sort + O(N) filter + O(N) bucketing). With stored derived properties, this is a regression vs the old computed-property approach which deferred work to view body evaluation. Fixed hot paths: - mergeAssistantAttention: 3–6 field writes → 1 writeback - markAllConversationsSeen: N element mutations → 1 snapshot writeback - restoreUnseen: N element mutations → 1 snapshot writeback - rollbackUnreadMutationIfNeeded: 2 field writes → 1 writeback - ConversationRestorer existing-conversation merge: 3 field writes + mergeAssistantAttention (4 total didSet triggers) → 1 writeback with inlined attention fields Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
…conciliation The previous commit inlined attention field assignments to avoid extra didSet triggers, but this bypassed the pendingAttentionOverrides logic in mergeAssistantAttention. This matters when a notification conversation is created and opened by the user before the session list response arrives — the user's local seen/unread state would be overwritten by stale server data. Now: non-attention fields (groupId, displayOrder, forkParent) use copy-modify-writeback (1 didSet), then mergeAssistantAttention handles attention fields with override reconciliation (1 more didSet). Total: 2 didSet triggers, down from 4 in the original code. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
- pinConversation: 2 per-field writes → 1 writeback - unpinConversation: 2 per-field writes → 1 writeback - moveConversationToGroup: 2-3 per-field writes → 1 writeback - handleAssistantMessageArrival: up to 3 per-field writes → 1 writeback - handleNotificationIntentForExistingConversation: up to 3 per-field writes → 1 writeback - performActivation(for:): add short-circuit guard when already-active conversation is re-activated, avoiding redundant VM creation, daemon notification, and observation wiring - ConversationRestorer groups assignment: wrap in withTransaction to suppress animations during the groups-only didSet Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
ashleeradka
approved these changes
Apr 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Eliminates a 2s+ main-thread hang during startup caused by the conversation restoration flow executing redundant activations, heavyweight property-observer side effects, and unnecessary SwiftUI animation computations synchronously. Also reduces ongoing sidebar recomputation cost by caching derived properties and using copy-modify-writeback for struct mutations.
Why
During startup,
handleConversationListResponserestores ~50 conversations from the daemon. The previous implementation had several compounding costs:activateConversation(firstVisible.id)triggered a full side-effect chain (VM creation, daemon HTTP call, Combine subscriptions), thenrestoreLastActiveConversation()could activate a different conversation — running the entire chain again.didSet:activeConversationId.didSetran ~50 lines of synchronous side effects. Per Apple's Observation framework guidance, property observers should perform lightweight bookkeeping; side effects belong in explicit methods.conversationsarray assignment triggered SwiftUI diffing and animation interpolation for every row. Apple'sTransactionAPI allows suppressing this viadisablesAnimations.groupedConversations,visibleConversations, etc. were computed on every view body access. Multiple views reading the same property within a single layout pass each re-ran O(N log N) sort + O(N) filter independently. Apple recommends pre-computing expensive derived values outside the view body.Changes
Eliminate double activation (
ConversationRestorer): Activation target (saved last-active → first visible → new) computed once and activated once.Decouple side effects from
activeConversationIddidSet (ConversationSelectionStore):didSetnow does only UserDefaults persistence and stale-anchor clearing. Heavy work (VM creation, daemon notification, observation wiring) moves to explicitperformActivation(for:)/performDeactivation(). Setter isprivate(set)— all callers go through these methods or theactivateConversation()facade.performActivation(for:)short-circuits when the target is already active.Suppress animations during bulk restoration (
ConversationRestorer): Bothgroupsandconversationsassignments wrapped inwithTransaction { disablesAnimations = true }per Apple'sTransactionAPI.Cache derived sidebar properties (
ConversationListStore):groupedConversations,visibleConversations,sortedGroups,unseenVisibleConversationCount,archivedConversationsconverted to storedprivate(set)properties, recomputed once perconversations/groupsmutation viadidSet. Early-returns whenconversationsis empty.Copy-modify-writeback for multi-field struct mutations (
ConversationListStore,ConversationRestorer,ConversationManager): With change ci: add web and platform CI/PR workflows #4, eachconversations[index].field = valuetriggersdidSet→recomputeDerivedProperties()(O(N log N)). All multi-mutation paths now copy the struct locally and write back once:markAllConversationsSeen,restoreUnseen): snapshot pattern — N mutations → 1 writebackmergeAssistantAttention,pinConversation,unpinConversation,moveConversationToGroup,rollbackUnreadMutationIfNeeded,handleAssistantMessageArrival,handleNotificationIntentForExistingConversation): copy-modify-writeback → 1didSeteachConversationRestorerexisting-conversation merge: non-attention fields → 1 writeback, thenmergeAssistantAttentionforpendingAttentionOverridesreconciliation → 1 more. Total: 2, down from 4.Fix
archiveConversationfallback selection (ConversationManager):visibleAfter/visibleBeforenow exclude.privateconversations, matchingvisibleConversationssemantics.Fix
closeConversationstale selection (ConversationManager): AddedperformDeactivation()fallback when no next conversation exists.Benefits
activeConversationIdsetter isprivate(set)with lightweightdidSet— side effects are explicit and discoverable viaperformActivation/performDeactivation. Cached derived properties make the write-triggers explicit.Safety
activeConversationId = ...call sites migrated toperformActivation(for:)/performDeactivation()(verified via grep). Theprivate(set)access control will surface any missed callers as compile errors.withTransaction { disablesAnimations }only suppresses animation interpolation; it does not skip view updates. Views still reflect the new state.resolveConversationId,backfillConversationId,unarchiveConversation) trigger exactly one recompute each and are unchanged.References
Review & Testing Checklist for Human
private(set)change onactiveConversationIdwill surface any missed callers as compile errors. This is the most important verification step.performActivation(for:)short-circuit guard — Verify no flow depends on re-activation when the conversation is already active (e.g., after VM eviction while a conversation remains selected). The guardconversationId != activeConversationIdskips VM creation, daemon notification, and observation wiring.Notes
import SwiftUIadded toConversationRestorer.swift(model-layer file) forTransaction/withTransaction. This is the Apple-recommended API but is a layering consideration.lastActiveConversationIdStringadded toMockConversationRestorerDelegateto satisfy the updatedConversationRestorerDelegateprotocol.ConversationManager.activeConversationIdfacade is now read-only. All activation routes throughactivateConversation()/enterDraftMode().activationTargetonly helps on reconnect within the same process. This matches prior behavior.Link to Devin session: https://app.devin.ai/sessions/3ea6d041e22243cda500f9884cc251e1
Requested by: @ashleeradka