Skip to content
Closed
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
13 changes: 8 additions & 5 deletions clients/macos/SCROLL_STRATEGY.md
Original file line number Diff line number Diff line change
Expand Up @@ -206,16 +206,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
99 changes: 77 additions & 22 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,103 @@ 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 {
/// Estimates whether the user message content will exceed the collapse
/// threshold when rendered. Decides the collapse state deterministically
/// from the model (text + attachments) rather than from observed geometry,
/// which avoids a measurement feedback loop: `.frame(height: 150)` forces
/// the observed subtree to lay out at 150pt, and if an `onGeometryChange`
/// inside that subtree writes the clamped height back to state, the
/// "is this message collapsible?" decision flips to false on the next
/// render — removing the "Show less" button and the clamp together.
///
/// Per-attachment heights mirror the renderers in `ChatBubbleAttachmentContent.swift`:
/// single images use `AspectFitImageView` (~200pt typical), grid tiles are 120pt,
/// videos/inline previews are ~200pt, and file chips are compact rows.
///
/// The estimate is intentionally conservative — it only needs to correctly
/// classify content above/below ~150pt. When the estimate is wrong on the
/// low side, the worst-case UX is a missing "Show more" button and the
/// content rendering at its natural height (no clipping), which matches
/// the existing non-collapsible user message behavior.
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)
// Match the font used by `MarkdownSegmentView` (VFont.chat = 16pt DM Sans)
// so the estimate tracks the actual rendered text height. Using a smaller
// system font under-counts height and misses messages that render >150pt.
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)

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

// `bubbleChrome` adds top+bottom padding around the entire subtree
// (see `bubbleChrome` for user messages: `EdgeInsets(top: VSpacing.md, bottom: VSpacing.md, ...)`),
// and the inner VStack separates sibling content sections by `VSpacing.sm`.
// These contribute to the rendered bubble height that `.frame(height: 150)`
// clamps, so the estimate must include them — otherwise messages with
// intrinsic content between (150 - padding) and 150pt render taller than
// the clamp but are classified as non-collapsible.
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
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in e9d78b7. The estimate now adds 2 * VSpacing.md (bubble chrome vertical padding) plus VSpacing.sm per inter-section gap in the inner VStack, so content between ~126pt and 150pt is now correctly classified as collapsible.


// MARK: - User Message Collapse / Expand
//
// .frame(maxHeight:) creates _FlexFrameLayout which recursively measures
// `.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.
// subtree — O(n × depth) per layout pass, causing 35 s+ hangs. We use
// `.frame(height:)` instead (`_FrameLayout` — O(1), no alignment cascade).
//
// 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 collapsibility decision is driven by a deterministic content
// estimate (see `estimatedContentExceedsCollapseThreshold`). Observing
// the clamped subtree's height and feeding it back into `@State` would
// create a layout/state feedback loop that collapses the button on
// toggle — see 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.

@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