diff --git a/clients/AGENTS.md b/clients/AGENTS.md index 964faf7e1e3..36103a7d47b 100644 --- a/clients/AGENTS.md +++ b/clients/AGENTS.md @@ -198,7 +198,8 @@ Prefer migrating the parent to `@Observable` so the bridge becomes unnecessary ( - **Keep modifier chains lean.** Every SwiftUI modifier wraps its content in a new view value. Long chains of redundant or duplicated modifiers deepen the view tree and increase diffing cost. Group related modifiers, remove redundant ones (e.g., don't set `.font` on every row when the parent already sets it), and extract heavily-modified subtrees into standalone `@ViewBuilder` methods. - **Scope observation narrowly.** Only observe the specific properties a view needs. Prefer granular `@Observable` properties or `withObservationTracking` over observing an entire store that publishes on unrelated changes. When a view reads `@Observable` properties from many different objects, extract logical sections (toolbar, overlays, content) into standalone child `View` structs — each child creates its own [observation tracking scope](https://developer.apple.com/videos/play/wwdc2023/10149/) so changes to one section's state don't re-evaluate the entire parent body. **Note:** private `@ViewBuilder` functions do NOT create separate observation scopes — they execute during the caller's `body` evaluation, so all their `@Observable` reads are tracked in the caller's scope. Use standalone `View` structs or [`ObservationBoundaryView`](https://developer.apple.com/videos/play/wwdc2023/10149/) (deferred `@escaping @ViewBuilder` closure) when extraction into a full `View` struct is impractical. - **Don't double-track scroll viewport geometry.** When an ancestor scroll container already publishes its visible height (e.g. via [`onScrollGeometryChange`](https://developer.apple.com/documentation/swiftui/view/onscrollgeometrychange(for:of:_:))), expose it to descendants through a file-local [`EnvironmentKey`](https://developer.apple.com/documentation/swiftui/environmentkey) instead of re-measuring with `containerRelativeFrame` + [`onGeometryChange`](https://developer.apple.com/documentation/swiftui/view/ongeometrychange(for:of:action:)) inside descendant views. Each independent `@State` viewport probe inside a large view hierarchy adds another layout pass that re-traverses the hierarchy on every viewport change. -- **Coalesce N writes into one when mutating `@Observable` stored properties in a loop.** When a stored property's `didSet` runs derived work (e.g. `ConversationListStore.conversations` triggers `recomputeDerivedProperties`), build a local `var snapshot = property`, mutate elements in the snapshot, and write `property = snapshot` once after the loop. N independent writes fire N `didSet` invocations — each doing the full derived-property recompute — scaling as O(N × work) on the main actor. The snapshot pattern collapses this to a single `didSet`. See `markConversationsSeenImpl`, `restoreUnseen`, `appendConversations`, and `deleteGroup` in `ConversationListStore.swift` for the reference implementations. When a value-level helper exists (e.g. `applyAssistantAttention(from:into:)`), prefer it over the single-row convenience wrapper so the caller retains control over when the writeback fires. +- **Coalesce N writes into one when mutating `@Observable` stored properties in a loop.** When a stored property's `didSet` runs derived work (e.g. `ConversationListStore.conversations` triggers `recomputeDerivedProperties`), build a local `var snapshot = property`, mutate elements in the snapshot, and write `property = snapshot` once after the loop. N independent writes fire N `didSet` invocations — each doing the full derived-property recompute — scaling as O(N × work) on the main actor. The snapshot pattern collapses this to a single `didSet`. See `markConversationsSeenImpl`, `restoreUnseen`, `appendConversations`, and `deleteGroup` in `ConversationListStore.swift` for the reference implementations. When a value-level helper exists (e.g. `applyAssistantAttention(from:into:)`, `applyLastInteracted(into:)`), prefer it over the single-row convenience wrapper so the caller retains control over when the writeback fires. +- **Guard `didSet` and writebacks against no-op mutations.** Array subscript assignment (`array[i] = value`) and whole-array assignment both fire `didSet` even when the new value equals the old. For properties whose `didSet` triggers expensive derived work, add `guard property != oldValue else { return }` in the `didSet`. For copy-modify-writeback sites, add `guard modified != original` before writing back. This eliminates redundant recomputes during high-frequency callbacks (e.g. streaming activity snapshots) where most invocations don't actually change the model. - **Cache derived booleans from high-frequency collections.** Reading `.isEmpty`, `.count`, `.first`, or `.contains` on an `@Observable` stored property [tracks the property, not the value](https://developer.apple.com/videos/play/wwdc2023/10149/) — any mutation invalidates the view even when the derived value hasn't changed, including reads inside `.onChange(of:)`. For frequently-mutated collections, maintain a separate stored scalar / lookup updated in `didSet` behind a `!=` guard, and route view-body and `.onChange` reads through the cached form. To prevent regression, mark the underlying collection [`@ObservationIgnored`](https://developer.apple.com/documentation/observation/observationignored()) so accidental view-body reads silently stop reacting (caught at code review) rather than working but expensive. - **Use `.task` + async sequences instead of `.onReceive` for `NotificationCenter`.** Each `.onReceive` modifier adds a generic wrapper layer to the view type and exists only for side effects — no rendering value. Use a single [`.task`](https://developer.apple.com/documentation/swiftui/view/task(priority:_:)) with [`NotificationCenter.notifications(named:)`](https://developer.apple.com/documentation/foundation/notificationcenter/notifications(named:object:)) async sequences instead. `.task` has identical lifecycle semantics (auto-cancelled on view disappear) without inflating the view type. - **Enable non-contiguous layout on TextKit 1 stacks in `NSViewRepresentable`.** When constructing an `NSLayoutManager` for an `NSTextView` hosted via `NSViewRepresentable`, set `layoutManager.allowsNonContiguousLayout = true`. The default (`false`) forces full-document glyph layout from character 0 on the main thread whenever a glyph range is queried — which happens the first time the text view is attached to its hosting view (`setDocumentView:` / `addSubview:` → `_setSuperview:` → `setNeedsDisplayInRect:` → `_glyphRangeForBoundingRect:`). Reference: [`NSLayoutManager.allowsNonContiguousLayout`](https://developer.apple.com/documentation/appkit/nslayoutmanager/allowsnoncontiguouslayout). **Exception — streaming text with a separate measurement stack:** when the representable measures height via a sibling TextKit stack that always fully lays out every glyph (e.g. `ensureLayout(for:)` + `usedRect(for:)`), the measured frame assumes the render stack will also lay out every glyph in that rect. Non-contiguous layout leaves glyphs pending until the view scrolls or draws them, which races with streaming updates — producing a gap (render paints a smaller laid-out region inside the correct frame) and then overlap (the next sibling is placed via the measured frame and the lazy glyphs later paint outside it). In that case set `allowsNonContiguousLayout = false` and document why. See `clients/shared/DesignSystem/Components/Display/SelectableTextView.swift` (lines 180–205) for the canonical example; `VCodeView` / `HighlightedTextView` still opt into non-contiguous layout independently because they do not share this measurement model. diff --git a/clients/macos/vellum-assistant/Features/MainWindow/ConversationListStore.swift b/clients/macos/vellum-assistant/Features/MainWindow/ConversationListStore.swift index 5f34bcb950f..b6e869e6467 100644 --- a/clients/macos/vellum-assistant/Features/MainWindow/ConversationListStore.swift +++ b/clients/macos/vellum-assistant/Features/MainWindow/ConversationListStore.swift @@ -55,7 +55,10 @@ final class ConversationListStore { /// /// Reference: [`@ObservationIgnored`](https://developer.apple.com/documentation/observation/observationignored()). @ObservationIgnored var conversations: [ConversationModel] = [] { - didSet { recomputeDerivedProperties() } + didSet { + guard conversations != oldValue else { return } + recomputeDerivedProperties() + } } var groups: [ConversationGroup] = [] { didSet { recomputeDerivedProperties() } @@ -531,11 +534,13 @@ final class ConversationListStore { func updateConversationTitle(id: UUID, title: String) { guard let index = conversations.firstIndex(where: { $0.id == id }) else { return } + guard conversations[index].title != title else { return } conversations[index].title = title } func updateConversationInferenceProfile(id: UUID, profile: String?) { guard let index = conversations.firstIndex(where: { $0.id == id }) else { return } + guard conversations[index].inferenceProfile != profile else { return } conversations[index].inferenceProfile = profile } @@ -546,6 +551,7 @@ final class ConversationListStore { let trimmed = title.trimmingCharacters(in: .whitespacesAndNewlines) guard !trimmed.isEmpty else { return } guard let index = conversations.firstIndex(where: { $0.id == id }) else { return } + guard conversations[index].title != trimmed else { return } conversations[index].title = trimmed if let conversationId = conversations[index].conversationId { Task { await conversationListClient.renameConversation(conversationId: conversationId, name: trimmed) } @@ -556,20 +562,28 @@ final class ConversationListStore { // MARK: - Recency - func updateLastInteracted(conversationId: UUID) { - guard let index = conversations.firstIndex(where: { $0.id == conversationId }) else { return } - var conversation = conversations[index] + /// Value-level helper: apply last-interacted timestamp to a local snapshot. + /// Returns `true` if a pin-change notification is needed (displayOrder was + /// cleared for a Recents conversation). Callers that process multiple + /// mutations can use this to coalesce writes into a single `conversations` + /// writeback — same pattern as `applyAssistantAttention(from:into:)`. + @discardableResult + func applyLastInteracted(into conversation: inout ConversationModel) -> Bool { conversation.lastInteractedAt = Date() if conversation.groupId == ConversationGroup.all.id { - // Clear explicit displayOrder so Recents conversations revert to recency-based sorting. - // Grouped conversations keep their manual ordering. let hadOrder = conversation.displayOrder != nil if hadOrder { conversation.displayOrder = nil } - conversations[index] = conversation - if hadOrder { sendPinChange(for: conversation) } - } else { - conversations[index] = conversation + return hadOrder } + return false + } + + func updateLastInteracted(conversationId: UUID) { + guard let index = conversations.firstIndex(where: { $0.id == conversationId }) else { return } + var conversation = conversations[index] + let needsPinChange = applyLastInteracted(into: &conversation) + conversations[index] = conversation + if needsPinChange { sendPinChange(for: conversation) } } // MARK: - Pinning & Ordering @@ -1210,6 +1224,7 @@ final class ConversationListStore { ) { var conversation = conversations[index] applyAssistantAttention(from: item, into: &conversation) + guard conversation != conversations[index] else { return } conversations[index] = conversation } diff --git a/clients/macos/vellum-assistant/Features/MainWindow/ConversationManager.swift b/clients/macos/vellum-assistant/Features/MainWindow/ConversationManager.swift index ebbdcf00783..877b45de336 100644 --- a/clients/macos/vellum-assistant/Features/MainWindow/ConversationManager.swift +++ b/clients/macos/vellum-assistant/Features/MainWindow/ConversationManager.swift @@ -1579,10 +1579,14 @@ final class ConversationManager: ConversationRestorerDelegate { } guard let index = listStore.conversations.firstIndex(where: { $0.id == conversationId }) else { return } let isNewMessage = previousSnapshot?.messageId != currentSnapshot.messageId - if isNewMessage && !listStore.conversations[index].isBackgroundConversation { - listStore.updateLastInteracted(conversationId: conversationId) - } + + // Snapshot-mutate-writeback: coalesce updateLastInteracted + attention + // mutations into a single conversations write to avoid 2× recompute. var conversation = listStore.conversations[index] + var needsPinChange = false + if isNewMessage && !conversation.isBackgroundConversation { + needsPinChange = listStore.applyLastInteracted(into: &conversation) + } if conversation.latestAssistantMessageAt == nil || isNewMessage { conversation.latestAssistantMessageAt = Date() } @@ -1599,6 +1603,7 @@ final class ConversationManager: ConversationRestorerDelegate { conversation.hasUnseenLatestAssistantMessage = true } listStore.conversations[index] = conversation + if needsPinChange { listStore.sendPinChange(for: conversation) } if shouldEmitSeenSignal, let daemonId = conversation.conversationId { listStore.emitConversationSeenSignal(conversationId: daemonId) } diff --git a/clients/macos/vellum-assistant/Features/MainWindow/ConversationRestorer.swift b/clients/macos/vellum-assistant/Features/MainWindow/ConversationRestorer.swift index cc5c1f60a01..ff1be089e8e 100644 --- a/clients/macos/vellum-assistant/Features/MainWindow/ConversationRestorer.swift +++ b/clients/macos/vellum-assistant/Features/MainWindow/ConversationRestorer.swift @@ -617,6 +617,7 @@ final class ConversationRestorer { func handleConversationTitleUpdated(_ response: ConversationTitleUpdatedMessage) { guard let delegate else { return } guard let index = delegate.conversations.firstIndex(where: { $0.conversationId == response.conversationId }) else { return } + guard delegate.conversations[index].title != response.title else { return } delegate.conversations[index].title = response.title }