Cache derived properties on ConversationListStore to eliminate observation cascades#29898
Conversation
🤖 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:
|
…ation cascades Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 784217c501
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /// equality guards. Mutations from `ConversationManager` and | ||
| /// `ConversationRestorer` continue to write through this property; the | ||
| /// `didSet` keeps the cached views in sync. | ||
| @ObservationIgnored var conversations: [ConversationModel] = [] { |
There was a problem hiding this comment.
Keep thread titles on an observable cache
With conversations now @ObservationIgnored, the still-existing ThreadWindowContentView.title computed property reads conversationManager.conversations, so a popped-out thread no longer subscribes to rename/backfill mutations; after renaming that conversation, the cached conversationsByLocalId readers update but the title bar can remain stale. Please migrate that title lookup (and other view-time reads) to an observable cached property before ignoring the backing array.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 381158f. ThreadWindowContentView.title now reads through conversationManager.listStore.conversationsByLocalId[conversationLocalId]?.title (the cached, observed lookup) instead of the now-@ObservationIgnored conversations array. Same migration applied to the lookup in popOutToWindow(_:) for consistency.
Swept the rest of the codebase in the same pass for any remaining direct view-body / drop-delegate reads of .conversations:
SidebarViewrow construction (pin-toggle action handler)SidebarSectionView(group-id lookup for drop targeting)SidebarSectionHeaderDropDelegate(validateDrop,performDrop,performConversationDrop)
All migrated to conversationsByLocalId for O(1) lookup and observation correctness. Resolving.
There was a problem hiding this comment.
🔴 ThreadWindowContentView.title reads @ObservationIgnored conversations in view body — title never updates on rename
The title computed property at ThreadWindow.swift:192-194 reads conversationManager.conversations, which forwards to listStore.conversations (ConversationManager.swift:109). Since this PR marks conversations as @ObservationIgnored (ConversationListStore.swift:53), SwiftUI's Observation framework no longer tracks this read during body evaluation. The title bar in pop-out thread windows will never reactively update when the conversation is renamed — directly contradicting the comment on line 190–191 which says it "updates reactively when the conversation is renamed, avoiding the stale-title bug."
The sibling conversation property at ThreadWindow.swift:351-353 was correctly migrated to use conversationsByLocalId (which IS observed), but title was missed in the same migration pass.
(Refers to lines 192-194)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed and fixed in 381158f. title now routes through conversationManager.listStore.conversationsByLocalId[conversationLocalId]?.title, which IS observed, so the title bar updates reactively on rename. The lookup in popOutToWindow(_:) was migrated in the same pass for consistency. Resolving.
…s review feedback Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
784217c to
381158f
Compare
There was a problem hiding this comment.
✦ APPROVE
Value: Eliminates multi-second main-thread hangs (MACOS-NK, -TG, -VR, -PR) for every v0.8.0 user — 8 events / 5 users active today. This is LUM-1429 plus 3 sibling Sentry fingerprints, all rooted in the same ConversationListStore.withMutation cascade.
What this does: Four-part cache-and-invalidate fix. (1) @ObservationIgnored on conversations eliminates the O(N²) observation cascade — every per-message seen-flip, heartbeat re-fetch, or pagination pulse was synchronously invalidating every subscriber. (2) Four new cached scalars (hasAnyConversations, hasAnyVisibleConversations, unseenScheduledCount, conversationsByLocalId lookup dict) give view bodies narrow observable targets instead of the full array. (3) != guards added to the four previously unguarded writes in recomputeDerivedProperties — specifically the sortedGroups, visibleConversations, unseenVisibleConversationCount, and groupedConversations writes that MACOS-TG's stack was landing in. (4) visibleNonArchivedConversationIds: Set<UUID> on ConversationSelectionStore — same pattern applied to the two selection-validation .contains scans in MainWindowView.
Checked against our KB:
- Follows the established
recomputeDerivedProperties+!=guards pattern frompatterns.md(Equality guards on @observable property writes). This PR extends it to the 4 cases that slipped through. - No new
withObservationTrackingloops. No.frame(maxWidth:). FlexFrame Lint ✅. @ObservationIgnoredcomments are thorough and accurate — doc-comment on the property cleanly documents the contract for future contributors.- Consumer sweep: all 7 direct
.conversationsreads in drop delegates and action handlers migrated toconversationsByLocalIdO(1) lookups ✅. All remaining.conversationsreferences inSidebarView/SidebarSectionView/ConversationSwitcherDrawerareSidebarGroupEntry.conversationsstruct fields (off the observablesidebarGroupEntriescache) — not rawlistStore.conversationsreads ✅. ConversationModel: Hashable→!=guard onconversationsByLocalId: [UUID: ConversationModel]is valid ✅.onDerivedPropertiesRecomputednot called from the standalonerecomputeArchivedConversationspath: intentional — archive-only mutations (timestamp dict changes) don't affectvisibleConversationsorvisibleNonArchivedConversationIds, so theselectionStoresync is correctly skipped.
Devin P1 / Codex P2 finding — both confirmed fixed at current HEAD (381158ffb8): Both bots independently caught that ThreadWindowContentView.title still read through conversationManager.conversations (now @ObservationIgnored), which would have permanently stalled the pop-out thread title bar on rename. Fixed: migrated to conversationManager.listStore.conversationsByLocalId[conversationLocalId]?.title. ✅
One robustness note (non-blocking): Dictionary(uniqueKeysWithValues:) will trap if conversations ever contains duplicate UUIDs. UUIDs are UUID() at creation so this is theoretical, but Dictionary(conversations.map { ($0.id, $0) }, uniquingKeysWith: { _, new in new }) would be the defensive form. Not a blocker — worth keeping in mind for the future.
Bot status: Devin's "4 additional findings" in the Devin Review UI aren't visible via the GitHub API — inline comments show all findings self-resolved at this SHA, but a quick check at https://app.devin.ai/review/vellum-ai/vellum-assistant/pull/29898 before merging is worth it. CI: Socket Security ✅, FlexFrame Lint ✅, macOS Build/Tests SKIPPED (expected).
Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
|
Thanks for the thorough review. Rolled the non-blocking robustness suggestion in as 923a79d — Left the two pre-existing |
…struct, terser cached-property docs, migrate remaining O(N) scans Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
Closes LUM-1429 and three sibling app-hangs (MACOS-NK, MACOS-TG, MACOS-VR, MACOS-PR) — multi-second main-thread hangs whose stacks land in
ConversationListStore.<setter>→withMutation→KeyPath._projectReadOnly.Why
The Observation framework tracks at the property level, not the value level. Every assignment to an
@Observablestored property fires thewillSethook and runs everywithObservationTrackingcallback synchronously, regardless of value equality. SwiftUI runs the resulting graph update inside that hook. When a view body or.onChange(of:)modifier reads anything off the liveconversationsarray (.isEmpty,.count { … },.contains { … },.first { … }), it subscribes to every mutation of the array — pagination, heartbeat re-fetch, per-message seen flip, in-place model edit — and stalls the main actor for the duration of the resulting graph update.The cache-and-invalidate pattern was already partially established but four direct subscribers slipped through. MACOS-TG's stack lands literally inside
recomputeDerivedProperties→groupedConversations.setter→withMutation, confirming the missing!=guards on its writes — the function's own comment documents why guards are required, but the discipline was applied torecomputeSidebarGroupEntriesonly.What
ConversationListStore—hasAnyConversations,hasAnyVisibleConversations,unseenScheduledCount, andconversationsByLocalId(O(1) lookup), all maintained inrecomputeDerivedProperties()behind!=guards.Set<UUID>onConversationSelectionStore—visibleNonArchivedConversationIdsfor selection-validation.containschecks, synced via the existingonDerivedPropertiesRecomputedcallback alongsidesyncActiveConversationCache.!=equality guards on the four unguarded writes inrecomputeDerivedProperties(sortedGroups,visibleConversations,unseenVisibleConversationCount,groupedConversations).@ObservationIgnoredonConversationListStore.conversationsso any future view-body read of the raw array is a silent no-op rather than an O(N²) anti-pattern. The doc-comment documents the rule.archivedConversationsconverted from computed → cached to satisfy step 4 for the Settings → Archive tab.DropDelegatemethods route through the cached scalars /conversationsByLocalIdlookup — replaces O(N) scans with O(1) and eliminates accidental observation subscriptions..first/.containsand the@ObservationIgnoredenforcement pattern.Benefits
KeyPath._projectReadOnlytraversal from the observation graph.first(where:),contains(where:)) to O(1) dictionary / set lookups across drag, drop, and per-conversation view paths.@ObservationIgnoredmakes the next instance of the anti-pattern a code-review issue rather than a production hang.Why it's safe
sidebarGroupEntries/systemSidebarGroupEntries/customSidebarGroupEntriesandactiveConversation. Cache freshness is guaranteed becauserecomputeDerivedPropertiesis the single mutation funnel and runs synchronously indidSet.conversations(assignment,append,insert,remove, subscript, in-place field edits) triggerdidSet.@ObservationIgnoredonly affects observation tracking, not setter semantics.archivedConversationsnow sorts on everyconversationsmutation rather than lazily on Settings open. For typical N this is dwarfed by the existingvisibleConversationssort in the same function and skipped on no-op writes by the!=guard.References
withObservationTracking(_:onChange:)@ObservationIgnoredView.onChange(of:)Alternatives considered and rejected
conversationssetter toTask { @MainActor in … }— moves the synchronous cost one tick later without addressing the root cause. The next mutation hits the same cascade.withTransaction(.init(animation: nil))/disablesAnimations— affects animations only. Does not suppress@Observablechange notifications,didSet, or the synchronous SwiftUI graph update insidewillSet.@ObservationIgnored(step 4) for safety — the same anti-pattern slipped past two prior fixes. Making accidental view-body reads a silent no-op (rather than working but expensive) lets the cached-scalar discipline lock in by default. The tradeoff: the next instance silently stops reacting rather than working but slow, which is caught at PR review rather than as a production hang. The doc-comment onconversationscalls out the rule explicitly.Set<UUID>onConversationListStoreinstead ofConversationSelectionStore— placement onConversationSelectionStorekeeps cross-store reads off the hot list-mutation path, matching the precedent set bysyncActiveConversationCache.Root cause analysis
@Observableinstance as if SwiftUI would diff by value — it diffs by property identity..onChange(of: collection.derivedScalar)reads as cheap inline computation, but the Observation framework registers a subscription on the collection itself.recomputeDerivedPropertieswrites; the function's own comment documented why guards were required, but the discipline was applied torecomputeSidebarGroupEntriesonly. Each new hang was triaged as a fresh issue rather than as the same root cause as the prior one.conversations@ObservationIgnoredmakes the next accidental view-body read a silent no-op rather than a working-but-expensive read, surfacing the issue at PR review rather than in production hang telemetry. The cached lookup and scalars are the only supported access paths from view bodies..first/.contains(the variants that were missed) and the@ObservationIgnoredenforcement pattern. Kept lightweight and link-heavy per project doc conventions.Reviewer checklist
conversationsByLocalId).Link to Devin session: https://app.devin.ai/sessions/0b91d3c9d59c46b19dd02266249309fb
Requested by: @ashleeradka