From f90fba8efdaf53715e06585eb5ef9cd899d6b2be Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Fri, 8 May 2026 19:08:22 +0000 Subject: [PATCH 1/3] fix(macos/chat): consolidate pinned-turn viewport into MessageListView env PinnedLatestTurnSection sized its topAlignedMinHeight floor against the scroll container by running its own containerRelativeFrame + onGeometryChange probe and mirroring the resolved height into a local @State. MessageListView already tracks the same value via OnScrollGeometryChange to drive bottomAlignedMinHeight. The probe's @State round-trip was redundant and added a third full layout pass through MessageTranscriptStack's eager VStack on every viewport change (composer Enter resize, splitter drag, window resize, conversation switch), surfacing as the LUM-1353 hang. Publish viewportHeight from MessageListView via a chat-local ScrollViewportHeightKey environment value (mirroring the BubbleMaxWidthKey pattern in ChatBubble.swift) and have PinnedLatestTurnSection read it through @Environment. Going through EnvironmentValues bypasses the MessageListContentView Equatable barrier without widening it: only descendants that read \.scrollViewportHeight re-evaluate on viewport changes. The VSpacing.md * 2 subtraction matches the outer MessageListContentView.body padding the probe was accounting for, and topAlignedMinHeight(nil) is already a no-op for the initial frame before OnScrollGeometryChange lands its first measurement. --- .../Chat/MessageListContentView.swift | 50 ++++++++----------- .../Features/Chat/MessageListView.swift | 30 +++++++++++ 2 files changed, 51 insertions(+), 29 deletions(-) diff --git a/clients/macos/vellum-assistant/Features/Chat/MessageListContentView.swift b/clients/macos/vellum-assistant/Features/Chat/MessageListContentView.swift index fd529c323d4..af8bd1c1226 100644 --- a/clients/macos/vellum-assistant/Features/Chat/MessageListContentView.swift +++ b/clients/macos/vellum-assistant/Features/Chat/MessageListContentView.swift @@ -457,13 +457,16 @@ 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`). Reading it + // here lets us size the section's `topAlignedMinHeight` floor against the + // viewport without a second `containerRelativeFrame` + `onGeometryChange` + // probe — which would feed an independent `@State` and add another full + // layout pass through the eager transcript stack on every viewport change. + // `nil` until `OnScrollGeometryChange` lands its first measurement, in + // which case `topAlignedMinHeight(nil)` is a no-op and the section sits + // at its natural height for the initial frame. + @Environment(\.scrollViewportHeight) private var scrollViewportHeight: CGFloat? private var hasResponseContent: Bool { contentView.showsStandaloneLatestEdgeActivity @@ -471,6 +474,17 @@ 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. + private var viewportMinHeight: CGFloat? { + scrollViewportHeight.map { max(0, $0 - VSpacing.md * 2) } + } + var body: some View { // Two flips (ScrollView + section) cancel out, so source order // equals visual order: anchor at top, response below, spacer @@ -504,31 +518,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..1787b79da46 100644 --- a/clients/macos/vellum-assistant/Features/Chat/MessageListView.swift +++ b/clients/macos/vellum-assistant/Features/Chat/MessageListView.swift @@ -5,6 +5,31 @@ 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 `OnScrollGeometryChange` +/// callback lands (e.g., during initial render or right after a +/// conversation switch resets the value). +/// +/// Published by `MessageListView` from its filtered `viewportHeight` state +/// so descendants — like `PinnedLatestTurnSection`'s `topAlignedMinHeight` +/// floor — can size against the viewport without measuring it again. +/// Going through `EnvironmentValues` (rather than props on the equatable +/// `MessageListContentView`) keeps the `.equatable()` barrier in place: +/// only descendants that read `\.scrollViewportHeight` re-evaluate when +/// the viewport changes; the rest of the transcript stays inert. +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 +221,11 @@ struct MessageListView: View { .environment(\.thinkingBlockExpansionStore, thinkingBlockExpansionStore) .environment(\.filePreviewExpansionStore, filePreviewExpansionStore) .environment(\.messageHeightCache, messageHeightCache) + // Same filtered value `bottomAlignedMinHeight` consumes one line up. + // `PinnedLatestTurnSection` reads it to size its `topAlignedMinHeight` + // floor instead of running its own `containerRelativeFrame` probe — + // see `ScrollViewportHeightKey` at the top of this file. + .environment(\.scrollViewportHeight, viewportHeight.isFinite ? viewportHeight : nil) .scrollIndicators(scrollState.scrollIndicatorsHidden ? .hidden : .automatic) .fixedWidth(widths.scrollSurfaceWidth) .id(conversationId) From 444807db6aafcde2cac681cdda7a7df50b8dd368 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Fri, 8 May 2026 19:14:11 +0000 Subject: [PATCH 2/3] fix(macos/chat): keep pinned-turn TopAlignedMinHeightLayout wrapping stable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit topAlignedMinHeight(_:) is a @ViewBuilder modifier that wraps content in TopAlignedMinHeightLayout when the value is non-nil and returns the content unwrapped when nil. Driving it from Optional meant the section rendered without the wrapper before the first OnScrollGeometryChange measurement landed, then SwiftUI flipped its structural identity to add the wrapper on the first non-nil value — rebuilding every transcript row inside the pinned turn on the initial frame. Fall back to 0 when scrollViewportHeight is nil so topAlignedMinHeight always wraps in TopAlignedMinHeightLayout (matching the old `@State viewportMinHeight: CGFloat = 0` default). Identity stays stable across the first measurement; minHeight: 0 has no sizing effect on content that's already non-negative. --- .../Features/Chat/MessageListContentView.swift | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/clients/macos/vellum-assistant/Features/Chat/MessageListContentView.swift b/clients/macos/vellum-assistant/Features/Chat/MessageListContentView.swift index af8bd1c1226..a1e972a1730 100644 --- a/clients/macos/vellum-assistant/Features/Chat/MessageListContentView.swift +++ b/clients/macos/vellum-assistant/Features/Chat/MessageListContentView.swift @@ -481,8 +481,18 @@ private struct PinnedLatestTurnSection: View { /// zero-height layout passes SwiftUI runs during setup and /// window/split-view collapse — negative minimums cause layout /// warnings and unstable pinned-turn rendering. - private var viewportMinHeight: CGFloat? { - scrollViewportHeight.map { max(0, $0 - VSpacing.md * 2) } + /// + /// Falls back to `0` (instead of `nil`) before the first + /// `OnScrollGeometryChange` measurement lands so `topAlignedMinHeight` + /// always wraps the section in `TopAlignedMinHeightLayout`. The + /// modifier's `@ViewBuilder` returns `self` unwrapped on `nil` and + /// `TopAlignedMinHeightLayout { self }` otherwise, so a `nil → value` + /// transition would change the section's structural identity and + /// rebuild every transcript row inside on the first measurement — + /// matching the old `@State viewportMinHeight: CGFloat = 0` default + /// keeps identity stable across that transition. + private var viewportMinHeight: CGFloat { + scrollViewportHeight.map { max(0, $0 - VSpacing.md * 2) } ?? 0 } var body: some View { From cd70cb2f654a9f28d377ceb79be8db3aaaf4a2fe Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Fri, 8 May 2026 19:47:11 +0000 Subject: [PATCH 3/3] docs/comments cleanup: remove PR-history references; add AGENTS rule --- clients/AGENTS.md | 1 + .../Chat/MessageListContentView.swift | 32 +++++++++---------- .../Features/Chat/MessageListView.swift | 27 ++++++++-------- 3 files changed, 29 insertions(+), 31 deletions(-) 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 a1e972a1730..d8d857b0abe 100644 --- a/clients/macos/vellum-assistant/Features/Chat/MessageListContentView.swift +++ b/clients/macos/vellum-assistant/Features/Chat/MessageListContentView.swift @@ -458,14 +458,13 @@ private struct PinnedLatestTurnSection: View { let thinkingLabel: String // Filtered scroll-container visible height published by `MessageListView` - // (see `ScrollViewportHeightKey` in `MessageListView.swift`). Reading it - // here lets us size the section's `topAlignedMinHeight` floor against the - // viewport without a second `containerRelativeFrame` + `onGeometryChange` - // probe — which would feed an independent `@State` and add another full - // layout pass through the eager transcript stack on every viewport change. - // `nil` until `OnScrollGeometryChange` lands its first measurement, in - // which case `topAlignedMinHeight(nil)` is a no-op and the section sits - // at its natural height for the initial frame. + // (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 { @@ -482,15 +481,14 @@ private struct PinnedLatestTurnSection: View { /// window/split-view collapse — negative minimums cause layout /// warnings and unstable pinned-turn rendering. /// - /// Falls back to `0` (instead of `nil`) before the first - /// `OnScrollGeometryChange` measurement lands so `topAlignedMinHeight` - /// always wraps the section in `TopAlignedMinHeightLayout`. The - /// modifier's `@ViewBuilder` returns `self` unwrapped on `nil` and - /// `TopAlignedMinHeightLayout { self }` otherwise, so a `nil → value` - /// transition would change the section's structural identity and - /// rebuild every transcript row inside on the first measurement — - /// matching the old `@State viewportMinHeight: CGFloat = 0` default - /// keeps identity stable across that transition. + /// 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 } diff --git a/clients/macos/vellum-assistant/Features/Chat/MessageListView.swift b/clients/macos/vellum-assistant/Features/Chat/MessageListView.swift index 1787b79da46..7a0a716d1ee 100644 --- a/clients/macos/vellum-assistant/Features/Chat/MessageListView.swift +++ b/clients/macos/vellum-assistant/Features/Chat/MessageListView.swift @@ -8,17 +8,16 @@ 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 `OnScrollGeometryChange` -/// callback lands (e.g., during initial render or right after a -/// conversation switch resets the value). +/// 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` state -/// so descendants — like `PinnedLatestTurnSection`'s `topAlignedMinHeight` -/// floor — can size against the viewport without measuring it again. -/// Going through `EnvironmentValues` (rather than props on the equatable -/// `MessageListContentView`) keeps the `.equatable()` barrier in place: -/// only descendants that read `\.scrollViewportHeight` re-evaluate when -/// the viewport changes; the rest of the transcript stays inert. +/// 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 } @@ -221,10 +220,10 @@ struct MessageListView: View { .environment(\.thinkingBlockExpansionStore, thinkingBlockExpansionStore) .environment(\.filePreviewExpansionStore, filePreviewExpansionStore) .environment(\.messageHeightCache, messageHeightCache) - // Same filtered value `bottomAlignedMinHeight` consumes one line up. - // `PinnedLatestTurnSection` reads it to size its `topAlignedMinHeight` - // floor instead of running its own `containerRelativeFrame` probe — - // see `ScrollViewportHeightKey` at the top of this file. + // 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)