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/macos/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
13 changes: 8 additions & 5 deletions clients/macos/SCROLL_STRATEGY.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
81 changes: 58 additions & 23 deletions clients/macos/vellum-assistant/Features/Chat/ChatBubble.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<Content: View>(@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) {
Expand Down