diff --git a/clients/macos/AGENTS.md b/clients/macos/AGENTS.md index e88e4ef511b..f9fd76710df 100644 --- a/clients/macos/AGENTS.md +++ b/clients/macos/AGENTS.md @@ -259,12 +259,7 @@ All design system types use the `V` prefix (VButton, VColor, VFont, etc.). Alway - [`.containerRelativeFrame(.horizontal)`](https://developer.apple.com/documentation/swiftui/view/containerrelativeframe(_:alignment:)) — width constraint without FlexFrame. Never trade `HStack+Spacer` for `.frame(alignment:)` in lazy containers — fewer layout nodes is not worth O(n) recursive alignment queries per node. -- **`AlignmentBarrierLayout` for LazyVStack protection**: Wrap the `ScrollView` (from the **outside**) with `AlignmentBarrierLayout { ScrollView { ... } }` to block `explicitAlignment` queries from cascading into the lazy container. The barrier returns `nil` for all alignment queries while passing through sizing and placement unchanged. Three critical constraints: - 1. **The barrier MUST wrap the ScrollView, not its content.** Placing a custom `Layout` _inside_ a `ScrollView` (between the `ScrollView` and a `LazyVStack`) breaks `LazyVStack`'s viewport tracking — cells fail to materialize on resize, send, or content changes until the user scrolls. Custom `Layout` containers disrupt the internal communication channel `ScrollView` uses to tell `LazyVStack` which cells are visible. - 2. **`.id()` MUST be outside the barrier, not inside.** A custom `Layout` container does not propagate child identity changes to the outer modifier chain. If `.id(conversationId)` is inside the `Layout`, `.onAppear`/`.onDisappear` on the outer chain will not fire on identity changes (e.g. conversation switch), breaking lifecycle handlers like `handleAppear()`. Place `.id()` after the barrier in the modifier chain: `AlignmentBarrierLayout { ScrollView { ... } }.id(conversationId)`. - 3. **Scroll-specific modifiers MUST be inside the barrier, on the ScrollView.** `.scrollPosition`, `.defaultScrollAnchor`, `.onScrollPhaseChange`, and `.scrollIndicators` must be applied directly to the `ScrollView` inside the barrier's content closure. Placing them outside forces SwiftUI to traverse the custom `Layout` container to find the `ScrollView`, which can intermittently fail and cause cells to not materialize. - - See [`Layout.explicitAlignment`](https://developer.apple.com/documentation/swiftui/layout/explicitalignment(of:in:proposal:subviews:cache:)-3iqmu) and `clients/shared/DesignSystem/Layout/AlignmentBarrierLayout.swift`. +- **Use `.frame(width:)` (not `.frame(maxWidth:)`) on ScrollView containers with LazyVStack**: `.frame(width:)` creates [`_FrameLayout`](https://developer.apple.com/documentation/swiftui/view/frame(width:height:alignment:)) which returns `bounds.midX` for alignment without querying children — the alignment cascade stops here. `.frame(maxWidth:)` creates [`_FlexFrameLayout`](https://developer.apple.com/documentation/swiftui/view/frame(minwidth:idealwidth:maxwidth:minheight:idealheight:maxheight:alignment:)) which queries `explicitAlignment` on children, cascading O(n) through the entire `LazyVStack` subtree on every layout pass. Use a computed width (e.g. `min(containerWidth, maxWidth)`) to get responsive behavior without `_FlexFrameLayout`. Do NOT use a custom `Layout` barrier — custom `Layout` containers between the outer modifier chain and the `ScrollView` disrupt SwiftUI's internal scroll infrastructure, causing intermittent cell materialization failures. - **Gallery**: When adding or modifying a design system primitive/component, update the corresponding Gallery section file (`Gallery/Sections/`) so the visual catalog stays current. - **Accessibility**: See `clients/AGENTS.md` § [Accessibility](../AGENTS.md#accessibility) for the full checklist (labels, hidden elements, custom interactions, AppKit panels). All rules there apply to macOS components. diff --git a/clients/macos/vellum-assistant/Features/Chat/MessageListView.swift b/clients/macos/vellum-assistant/Features/Chat/MessageListView.swift index 43c25efdc3b..e706a99483b 100644 --- a/clients/macos/vellum-assistant/Features/Chat/MessageListView.swift +++ b/clients/macos/vellum-assistant/Features/Chat/MessageListView.swift @@ -110,70 +110,53 @@ struct MessageListView: View { #if DEBUG let _ = os_signpost(.event, log: PerfSignposts.log, name: "MessageListView.body") #endif - // AlignmentBarrierLayout stops explicitAlignment queries from the - // outer .frame(maxWidth:) modifiers from cascading into the - // LazyVStack. It MUST wrap the ScrollView from the outside — - // placing it inside the ScrollView breaks LazyVStack's viewport - // tracking, causing cells to not materialize until scrolled. - // See AGENTS.md and AlignmentBarrierLayout.swift for details. - // - // Scroll-specific modifiers (.scrollPosition, .defaultScrollAnchor, - // .onScrollPhaseChange, .scrollIndicators) are applied INSIDE the - // barrier directly on the ScrollView. Placing them outside forces - // SwiftUI to traverse a custom Layout container to find the - // ScrollView, which can intermittently fail and cause cells to - // not materialize. - AlignmentBarrierLayout { - ScrollView { - scrollViewContent - .background( - MessageListScrollObserver { newState in - enqueueScrollGeometryUpdate(newState) - } - ) - } - .scrollContentBackground(.hidden) - .scrollDisabled(messages.isEmpty && !isSending) - // Apply only to .initialOffset — where the scroll view starts - // when first displayed (including .id() recreation on switch). - // Deliberately NOT using the all-roles overload (.sizeChanges) - // because it fights user scroll-up during streaming: SwiftUI's - // definition of "at bottom" for anchor purposes can differ from - // our hysteresis-based isAtBottom, causing the viewport to snap - // back to bottom on every content-height change even after the - // user has entered freeBrowsing. Our explicit content-height - // auto-follow handles streaming growth with proper mode checks. - // https://developer.apple.com/documentation/swiftui/view/defaultscrollanchor(_:for:) - .defaultScrollAnchor(.bottom, for: .initialOffset) - .scrollPosition($scrollPosition) - .environment(\.suppressAutoScroll, { [self] in - os_signpost(.event, log: PerfSignposts.log, name: "scrollSuppressionChanged", "on reason=manualExpansionDetach") - let intents = scrollCoordinator.handle(.manualExpansion) - executeCoordinatorIntents(intents) - // Keep scrollState in sync as runtime executor. - scrollState.handleManualExpansionInteraction() - }) - .onScrollPhaseChange { oldPhase, newPhase in - let coordinatorPhase = ScrollCoordinator.Phase.from(newPhase) - let intents = scrollCoordinator.handle(.scrollPhaseChanged(phase: coordinatorPhase)) - executeCoordinatorIntents(intents) - // Keep scrollState in sync as runtime executor. - scrollState.scrollPhase = newPhase - if newPhase == .idle && oldPhase != .idle && scrollState.isAtBottom { - scrollState.handleReachedBottom() - } + // .frame(width:) creates _FrameLayout (not _FlexFrameLayout). FrameLayout + // returns bounds.midX for alignment without querying children, stopping the + // alignment cascade. The old .frame(maxWidth:) pattern created FlexFrameLayout + // which queried explicitAlignment on the entire LazyVStack subtree — O(n) per + // layout pass, causing 34-70s hangs. See AGENTS.md. + ScrollView { + scrollViewContent + .background( + MessageListScrollObserver { newState in + enqueueScrollGeometryUpdate(newState) + } + ) + } + .scrollContentBackground(.hidden) + .scrollDisabled(messages.isEmpty && !isSending) + // Apply only to .initialOffset — where the scroll view starts + // when first displayed (including .id() recreation on switch). + // Deliberately NOT using the all-roles overload (.sizeChanges) + // because it fights user scroll-up during streaming: SwiftUI's + // definition of "at bottom" for anchor purposes can differ from + // our hysteresis-based isAtBottom, causing the viewport to snap + // back to bottom on every content-height change even after the + // user has entered freeBrowsing. Our explicit content-height + // auto-follow handles streaming growth with proper mode checks. + // https://developer.apple.com/documentation/swiftui/view/defaultscrollanchor(_:for:) + .defaultScrollAnchor(.bottom, for: .initialOffset) + .scrollPosition($scrollPosition) + .environment(\.suppressAutoScroll, { [self] in + os_signpost(.event, log: PerfSignposts.log, name: "scrollSuppressionChanged", "on reason=manualExpansionDetach") + let intents = scrollCoordinator.handle(.manualExpansion) + executeCoordinatorIntents(intents) + // Keep scrollState in sync as runtime executor. + scrollState.handleManualExpansionInteraction() + }) + .onScrollPhaseChange { oldPhase, newPhase in + let coordinatorPhase = ScrollCoordinator.Phase.from(newPhase) + let intents = scrollCoordinator.handle(.scrollPhaseChanged(phase: coordinatorPhase)) + executeCoordinatorIntents(intents) + // Keep scrollState in sync as runtime executor. + scrollState.scrollPhase = newPhase + if newPhase == .idle && oldPhase != .idle && scrollState.isAtBottom { + scrollState.handleReachedBottom() } - .scrollIndicators(scrollState.scrollIndicatorsHidden ? .hidden : .automatic) } - // .id() MUST be outside AlignmentBarrierLayout. A custom Layout - // container does not propagate child identity changes to the outer - // modifier chain — .onAppear would not fire on conversation switch - // if .id() were inside the Layout, breaking handleAppear() and all - // scroll state that depends on it (configureScrollCallbacks, - // handleConversationSwitched). See MessageListView+Lifecycle.swift. + .scrollIndicators(scrollState.scrollIndicatorsHidden ? .hidden : .automatic) .id(conversationId) - .frame(maxWidth: VSpacing.chatColumnMaxWidth) - .frame(maxWidth: .infinity) + .frame(width: containerWidth > 0 ? min(containerWidth, VSpacing.chatColumnMaxWidth) : VSpacing.chatColumnMaxWidth) .overlay(alignment: .bottom) { ScrollToLatestOverlayView(scrollState: scrollState) } diff --git a/clients/shared/DesignSystem/Layout/AlignmentBarrierLayout.swift b/clients/shared/DesignSystem/Layout/AlignmentBarrierLayout.swift deleted file mode 100644 index 115e813ee94..00000000000 --- a/clients/shared/DesignSystem/Layout/AlignmentBarrierLayout.swift +++ /dev/null @@ -1,51 +0,0 @@ -import SwiftUI - -/// Blocks [`explicitAlignment`](https://developer.apple.com/documentation/swiftui/layout/explicitalignment(of:in:proposal:subviews:cache:)-3iqmu) -/// queries from cascading into its subtree. Returns `nil` for both horizontal -/// and vertical alignment guides while passing sizing and placement through unchanged. -/// -/// Place between any `.frame(maxWidth:, alignment:)` and a `LazyVStack` to -/// prevent O(depth × children) recursive alignment measurement. -/// -/// - SeeAlso: [WWDC23 – Demystify SwiftUI performance](https://developer.apple.com/videos/play/wwdc2023/10160/) -/// - SeeAlso: [`ViewDimensions`](https://developer.apple.com/documentation/swiftui/viewdimensions) -public struct AlignmentBarrierLayout: Layout { - public init() {} - - public func sizeThatFits( - proposal: ProposedViewSize, - subviews: Subviews, - cache: inout () - ) -> CGSize { - subviews.first?.sizeThatFits(proposal) ?? .zero - } - - public func placeSubviews( - in bounds: CGRect, - proposal: ProposedViewSize, - subviews: Subviews, - cache: inout () - ) { - subviews.first?.place(at: bounds.origin, proposal: proposal) - } - - public func explicitAlignment( - of guide: HorizontalAlignment, - in bounds: CGRect, - proposal: ProposedViewSize, - subviews: Subviews, - cache: inout () - ) -> CGFloat? { - nil - } - - public func explicitAlignment( - of guide: VerticalAlignment, - in bounds: CGRect, - proposal: ProposedViewSize, - subviews: Subviews, - cache: inout () - ) -> CGFloat? { - nil - } -}