Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clients/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -457,20 +457,42 @@ 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
|| !contentView.state.orphanSubagents.isEmpty
|| !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
Expand Down Expand Up @@ -504,31 +526,9 @@ private struct PinnedLatestTurnSection: View {
contentView.latestEdgeSentinel(isFlipped: false)
}
.topAlignedMinHeight(viewportMinHeight)
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
.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) {
Expand Down
29 changes: 29 additions & 0 deletions clients/macos/vellum-assistant/Features/Chat/MessageListView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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)
Expand Down