diff --git a/clients/macos/AGENTS.md b/clients/macos/AGENTS.md index 55ca81289ed..8cbb0a3ff3f 100644 --- a/clients/macos/AGENTS.md +++ b/clients/macos/AGENTS.md @@ -273,6 +273,7 @@ All design system types use the `V` prefix (VButton, VColor, VFont, etc.). Alway - **Background**: Use a single `.background { }` with a `ZStack` inside instead of chaining multiple `.background()` calls. - **No-op backgrounds**: Never add invisible backgrounds like `.background(Capsule().fill(Color.clear))` — they create layout wrappers with zero visual effect. - **No animated insertions in chat `LazyVStack`**: ANY animated insertion/removal in a `LazyVStack` triggers `motionVectors` — an O(n) `sizeThatFits` measurement over ALL children that defeats lazy loading and causes multi-minute hangs. The chat message list uses `.transaction { $0.animation = nil }` to suppress all insertion animations. Do NOT remove that modifier or wrap content mutations in `withAnimation` that flows into the `LazyVStack`. See [`.transaction` docs](https://developer.apple.com/documentation/swiftui/view/transaction(_:)) and [WWDC23: Demystify SwiftUI performance](https://developer.apple.com/videos/play/wwdc2023/10160/). +- **Geometry observations must not drive state that changes the observed layout**: if a subtree is size-constrained (e.g., `.frame(height:)`, `.clipped()`) and an `onGeometryChange` or `GeometryReader` inside it writes the measured height/width into `@State` that gates the same constraint, you get a feedback loop — the observed value is the *clamped* value, so the decision to clamp flips off, the frame is removed, the child re-measures larger, the decision flips back on, and the layout oscillates or settles incorrectly. Derive layout-gating decisions from the model (content counts, text length, attachment types) or from a container-level geometry source that is *not* inside the constrained subtree. See [`onGeometryChange` docs](https://developer.apple.com/documentation/swiftui/view/ongeometrychange(for:of:action:)). - **No `.frame(maxWidth:)`, `.frame(maxHeight:)`, or `.frame(maxWidth:maxHeight:alignment:)` inside LazyVStack/LazyHStack/LazyVGrid cell hierarchy**: These create [`_FlexFrameLayout`](https://developer.apple.com/documentation/swiftui/view/frame(minwidth:idealwidth:maxwidth:minheight:idealheight:maxheight:alignment:)) whose `placement()` queries each child's explicit alignment via [`ViewDimensions.subscript`](https://developer.apple.com/documentation/swiftui/viewdimensions). Nested FlexFrames recurse O(depth × children) per layout pass. **This applies to ALL `maxWidth`/`maxHeight` values, not just `.infinity`** — bounded values like `.frame(maxWidth: 360)` still create `_FlexFrameLayout` and trigger the alignment cascade. The difference is magnitude, not kind. See [WWDC23: Demystify SwiftUI performance](https://developer.apple.com/videos/play/wwdc2023/10160/). Safe alternatives: - **`.widthCap(N)`** — uses `WidthCapLayout` ([Layout protocol](https://developer.apple.com/documentation/swiftui/layout), O(1)), caps width without creating `_FlexFrameLayout`. See `WidthCapLayout.swift`. Used in 10+ places in the codebase (PRs #24589, #26007, #26092). - `.frame(width: exactWidth)` — [`_FrameLayout`](https://developer.apple.com/documentation/swiftui/view/frame(width:height:alignment:)), no alignment query. diff --git a/clients/macos/SCROLL_STRATEGY.md b/clients/macos/SCROLL_STRATEGY.md index 385cebeda48..6bec77154d5 100644 --- a/clients/macos/SCROLL_STRATEGY.md +++ b/clients/macos/SCROLL_STRATEGY.md @@ -200,16 +200,19 @@ Deep-link scroll uses `.center` anchor for scroll-to-ID via `ScrollPosition` val --- -## User Message Collapse (Prevents First-Frame Flash) +## User Message Collapse -Long user messages collapse at 150pt. The collapse decision uses `NSString.boundingRect` on the first frame (before `onGeometryChange` fires) to avoid a full-height flash: +Long user messages collapse at 150pt. The collapse decision is driven **purely** by a deterministic estimate of text + per-attachment heights, computed from the model: ```swift -let isCollapsible = userMessageIntrinsicHeight > 0 - ? userMessageIntrinsicHeight > userMessageMaxCollapsedHeight - : estimatedTextExceedsCollapseThreshold // NSString.boundingRect estimate +let isCollapsible = estimatedContentExceedsCollapseThreshold +let needsCollapse = isCollapsible && !isUserMessageExpanded ``` +`estimatedContentExceedsCollapseThreshold` combines `NSString.boundingRect` on the message text with conservative per-attachment heights (single image ~200pt, grid tiles 120pt, videos/inline previews ~200pt, audio ~80pt, file chips ~40pt) that mirror the renderers in `ChatBubbleAttachmentContent.swift`. + +**Do not wire `onGeometryChange` (or any layout observation) into the collapsibility state.** `.frame(height: 150)` is a `_FrameLayout` that hard-proposes 150pt to its child; observing the child's height and feeding it back into state creates a feedback loop that flips `isCollapsible` to false on the first toggle, removing the "Show less" button and the frame clamp together. See [`onGeometryChange` docs](https://developer.apple.com/documentation/swiftui/view/ongeometrychange(for:of:action:)) for the general guidance that geometry observations should not drive state that changes the observed layout. + Collapsed messages have: - Gradient fade overlay (transparent -> `VColor.surfaceLift`) - "Show more" button using `VButton(style: .ghost, size: .compact, tintColor: .contentTertiary)`, left-aligned diff --git a/clients/macos/vellum-assistant/Features/Chat/ChatBubble.swift b/clients/macos/vellum-assistant/Features/Chat/ChatBubble.swift index 37a2f76a053..6f792feb13e 100644 --- a/clients/macos/vellum-assistant/Features/Chat/ChatBubble.swift +++ b/clients/macos/vellum-assistant/Features/Chat/ChatBubble.swift @@ -71,7 +71,6 @@ struct ChatBubble: View, Equatable { var isLatestAssistantMessage: Bool = false var typographyGeneration: Int = 0 @State private var isUserMessageExpanded: Bool = false - @State private var userMessageIntrinsicHeight: CGFloat = 0 private let userMessageMaxCollapsedHeight: CGFloat = 150 private static let heuristicUserCollapseCharacterThreshold = 3_000 private static let heuristicUserCollapseLineThreshold = 40 @@ -609,47 +608,83 @@ struct ChatBubble: View, Equatable { return preview == trimmedText ? preview : "\(preview)\n\n..." } - /// Estimates whether the user message text will exceed the collapse - /// threshold when rendered. Used on the first frame before - /// `onGeometryChange` has fired to avoid a full-height flash. - private var estimatedTextExceedsCollapseThreshold: Bool { + /// Conservative estimate of whether the rendered user message will exceed + /// `userMessageMaxCollapsedHeight`. Decision is derived from the model + /// (text + attachments) — never from observed geometry — because the + /// wrapper uses `.frame(height:)` to clamp the subtree, and observing the + /// clamped child's height would create a state/layout feedback loop. + /// See [onGeometryChange](https://developer.apple.com/documentation/swiftui/view/ongeometrychange(for:of:action:)). + /// + /// Underestimates degrade gracefully: no "Show more" button, content + /// renders at natural height (same as non-collapsible messages). + private var estimatedContentExceedsCollapseThreshold: Bool { guard isUser, !message.isStreaming else { return false } + let text = message.text as NSString let contentWidth = max(bubbleMaxWidth - 2 * VSpacing.lg, 0) - let font = NSFont.systemFont(ofSize: 14, weight: .regular) + // Must match the font used by MarkdownSegmentView to track rendered height. + let font = VFont.nsChat let textRect = text.boundingRect( with: NSSize(width: contentWidth, height: .greatestFiniteMagnitude), options: [.usesLineFragmentOrigin, .usesFontLeading], attributes: [.font: font] ) - return ceil(textRect.height) > userMessageMaxCollapsedHeight + let textHeight = ceil(textRect.height) + + // Attachment heights mirror renderers in ChatBubbleAttachmentContent.swift. + let parts = partitionedAttachments + let imageCount = parts.images.count + let imageHeight: CGFloat + if imageCount == 0 { + imageHeight = 0 + } else if imageCount == 1 { + imageHeight = 200 + } else { + let rows = ceil(Double(imageCount) / 2) + imageHeight = CGFloat(rows) * 120 + } + let videoHeight = CGFloat(parts.videos.count) * 200 + let audioHeight = CGFloat(parts.audios.count) * 80 + let fileHeight = CGFloat(parts.files.count) * 40 + + // Include bubble chrome padding and inter-section VStack spacing: both + // contribute to the rendered height that .frame(height: 150) clamps. + let bubbleVerticalPadding: CGFloat = 2 * VSpacing.md + let contentSections = [ + textHeight > 0, + imageHeight > 0, + videoHeight > 0, + audioHeight > 0, + fileHeight > 0 + ].filter { $0 }.count + let interSectionSpacing = CGFloat(max(0, contentSections - 1)) * VSpacing.sm + + let totalHeight = textHeight + + imageHeight + + videoHeight + + audioHeight + + fileHeight + + bubbleVerticalPadding + + interSectionSpacing + return totalHeight > userMessageMaxCollapsedHeight } // MARK: - User Message Collapse / Expand // - // .frame(maxHeight:) creates _FlexFrameLayout which recursively measures - // children and resolves explicitAlignment through the entire LazyVStack - // subtree — O(n × depth) per layout pass, causing 35 s+ hangs. + // `.frame(height:)` is load-bearing — `.frame(maxHeight:)` creates + // `_FlexFrameLayout`, which triggers the O(n × depth) alignment cascade + // through the LazyVStack subtree (35s+ hangs). See AGENTS.md. // - // Fix: .frame(height:) creates _FrameLayout — O(1), no alignment cascade. - // When height is nil (expanded / short), _FrameLayout passes through the - // child's natural height. Single view identity is preserved (no - // _ConditionalContent), so withAnimation still drives a smooth height - // transition on expand/collapse. + // The collapse decision must come from the model, not from observed + // geometry — the frame clamps the child, so feeding the child's height + // back into @State would create a state/layout feedback loop. @ViewBuilder private func userMessageHeightWrapper(@ViewBuilder _ content: () -> Content) -> some View { - let isCollapsible = userMessageIntrinsicHeight > 0 - ? userMessageIntrinsicHeight > userMessageMaxCollapsedHeight - : estimatedTextExceedsCollapseThreshold + let isCollapsible = estimatedContentExceedsCollapseThreshold let needsCollapse = isCollapsible && !isUserMessageExpanded VStack(alignment: .leading, spacing: 0) { content() - .onGeometryChange(for: CGFloat.self) { proxy in - proxy.size.height - } action: { height in - userMessageIntrinsicHeight = height - } .frame(height: needsCollapse ? userMessageMaxCollapsedHeight : nil, alignment: .top) .clipped() .overlay(alignment: .bottom) {