diff --git a/clients/AGENTS.md b/clients/AGENTS.md index 44af4f8a7b6..fdae3802626 100644 --- a/clients/AGENTS.md +++ b/clients/AGENTS.md @@ -197,6 +197,7 @@ Prefer migrating the parent to `@Observable` so the bridge becomes unnecessary ( - **Lazy containers for large collections.** Use `LazyVStack`, `LazyHStack`, `LazyVGrid` instead of eager equivalents when the item count is unbounded or large. In particular, avoid `VStack`/`HStack` inside a `ScrollView` for large or unbounded data-driven lists — eager loading kills scroll performance. Small, fixed-size lists where visibility-sensitive logic (e.g., `onAppear` pagination triggers) matters may use eager containers intentionally. See `clients/macos/AGENTS.md` §§ "No `.frame(maxWidth:)` …" for the FlexFrame anti-pattern that compounds with lazy containers (mechanically enforced in CI via `clients/scripts/check-flexframe.sh`). - **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. - **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/Chat/MessageListContentView.swift b/clients/macos/vellum-assistant/Features/Chat/MessageListContentView.swift index fd529c323d4..d8d857b0abe 100644 --- a/clients/macos/vellum-assistant/Features/Chat/MessageListContentView.swift +++ b/clients/macos/vellum-assistant/Features/Chat/MessageListContentView.swift @@ -457,13 +457,15 @@ private struct PinnedLatestTurnSection: View { let isUnanchoredThinking: Bool let thinkingLabel: String - // Minimum height equal to the scroll container's visible height - // (minus the outer LazyVStack's vertical padding). Sourced from a - // zero-size `containerRelativeFrame` probe in the `.background` so - // the section grows to fill the viewport when content is short but - // is still free to exceed it when an assistant response is longer - // than a viewport — preserving scrollability for tall answers. - @State private var viewportMinHeight: CGFloat = 0 + // Filtered scroll-container visible height published by `MessageListView` + // (see `ScrollViewportHeightKey` in `MessageListView.swift`). Used to + // size the section's `topAlignedMinHeight` floor against the viewport. + // `nil` until the first scroll-geometry callback lands. + // + // Read via `@Environment` so the value reaches us without going through + // `MessageListContentView`'s `Equatable` props — keeps the `.equatable()` + // barrier upstream intact while still letting this section react. + @Environment(\.scrollViewportHeight) private var scrollViewportHeight: CGFloat? private var hasResponseContent: Bool { contentView.showsStandaloneLatestEdgeActivity @@ -471,6 +473,26 @@ private struct PinnedLatestTurnSection: View { || !partition.responseItems.isEmpty } + /// Viewport-sized floor for the section, minus the outer transcript + /// padding (`VSpacing.md` top + `VSpacing.md` bottom in + /// `MessageListContentView.body`'s `EdgeInsets`) so the anchor row + /// lands with the intended visual gap. `max(0, …)` guards transient + /// zero-height layout passes SwiftUI runs during setup and + /// window/split-view collapse — negative minimums cause layout + /// warnings and unstable pinned-turn rendering. + /// + /// Defaults to `0` (not `nil`) before the first scroll measurement + /// lands so `topAlignedMinHeight` always wraps the section in + /// `TopAlignedMinHeightLayout`. The modifier is `@ViewBuilder` with + /// `if let minHeight`, so a `nil → value` transition would flip the + /// section's structural identity and rebuild every transcript row + /// inside on the first measurement; pinning the initial value to + /// `0` (sizing-equivalent to no floor since content is non-negative) + /// keeps identity stable. + private var viewportMinHeight: CGFloat { + scrollViewportHeight.map { max(0, $0 - VSpacing.md * 2) } ?? 0 + } + var body: some View { // Two flips (ScrollView + section) cancel out, so source order // equals visual order: anchor at top, response below, spacer @@ -504,31 +526,9 @@ private struct PinnedLatestTurnSection: View { contentView.latestEdgeSentinel(isFlipped: false) } .topAlignedMinHeight(viewportMinHeight) - .background(viewportMinHeightProbe) .flipped() } - /// Zero-width `Color.clear` sized to the scroll container's visible - /// height via `containerRelativeFrame`. `onGeometryChange` mirrors the - /// resolved height into `@State` so the VStack's `minHeight` tracks - /// the viewport. `max(0, …)` keeps the closure non-negative for the - /// transient zero-height layout passes SwiftUI runs during setup and - /// window/split-view collapse — negative frame dimensions produce - /// layout warnings and unstable pinned-turn rendering. - private var viewportMinHeightProbe: some View { - Color.clear - .containerRelativeFrame(.vertical, alignment: .top) { length, _ in - max(0, length - VSpacing.md * 2) - } - .onGeometryChange(for: CGFloat.self) { proxy in - proxy.size.height - } action: { newHeight in - if abs(viewportMinHeight - newHeight) > 0.5 { - viewportMinHeight = newHeight - } - } - } - @ViewBuilder private var responseCluster: some View { VStack(alignment: .leading, spacing: VSpacing.md) { diff --git a/clients/macos/vellum-assistant/Features/Chat/MessageListView.swift b/clients/macos/vellum-assistant/Features/Chat/MessageListView.swift index 3cf85d286d7..7a0a716d1ee 100644 --- a/clients/macos/vellum-assistant/Features/Chat/MessageListView.swift +++ b/clients/macos/vellum-assistant/Features/Chat/MessageListView.swift @@ -5,6 +5,30 @@ import os.signpost import SwiftUI import VellumAssistantShared +// MARK: - Scroll Viewport Height Environment + +/// The current visible height of the scroll container that hosts the +/// transcript, in points. `nil` until the first scroll-geometry callback +/// lands (initial render, or right after a conversation switch resets +/// the value). +/// +/// Published by `MessageListView` from its filtered `viewportHeight` so +/// descendants can size against the viewport without taking their own +/// measurement. Routing through `EnvironmentValues` rather than as a +/// prop on the equatable `MessageListContentView` preserves its +/// `.equatable()` barrier — only descendants that read +/// `\.scrollViewportHeight` re-evaluate on viewport changes. +private struct ScrollViewportHeightKey: EnvironmentKey { + static let defaultValue: CGFloat? = nil +} + +extension EnvironmentValues { + var scrollViewportHeight: CGFloat? { + get { self[ScrollViewportHeightKey.self] } + set { self[ScrollViewportHeightKey.self] = newValue } + } +} + struct MessageListView: View { let messages: [ChatMessage] @@ -196,6 +220,11 @@ struct MessageListView: View { .environment(\.thinkingBlockExpansionStore, thinkingBlockExpansionStore) .environment(\.filePreviewExpansionStore, filePreviewExpansionStore) .environment(\.messageHeightCache, messageHeightCache) + // Publish the same filtered viewport height `bottomAlignedMinHeight` + // consumes so descendant sections can size against the viewport + // without taking their own measurement. See + // `ScrollViewportHeightKey` at the top of this file. + .environment(\.scrollViewportHeight, viewportHeight.isFinite ? viewportHeight : nil) .scrollIndicators(scrollState.scrollIndicatorsHidden ? .hidden : .automatic) .fixedWidth(widths.scrollSurfaceWidth) .id(conversationId)