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/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ Prefer migrating the parent to `@Observable` so the bridge becomes unnecessary (
- **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.
- **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.
- **Prefer line-count math over `ensureLayout(for:)` for `sizeThatFits` when line wrapping is disabled and line height is pinned.** If the TextKit 1 stack uses an unbounded-width `NSTextContainer` (so lines never wrap) and the paragraph style pins `minimumLineHeight == maximumLineHeight`, the total height is exactly `lineCount * lineHeight + insets` — no glyph layout required. This avoids an O(glyph count) main-thread layout pass that SwiftUI would otherwise re-run on every cell during `LazyVStack` layout. Reference: [`NSLayoutManager.defaultLineHeight(for:)`](https://developer.apple.com/documentation/appkit/nslayoutmanager/defaultlineheight(for:)).
- **Decouple `NSTextContainer` from the view frame when an `NSViewRepresentable` hands SwiftUI a precomputed size.** If the caller applies `.frame(width:height:)` from a static measurement helper (`measureSize`), set [`NSTextContainer.widthTracksTextView`](https://developer.apple.com/documentation/uikit/nstextcontainer/widthtrackstextview) to `false`, size the container explicitly from the same `maxWidth` used to measure, and propagate `maxWidth` changes onto `containerSize` in `updateNSView`. Do not pair this path with `autoresizingMask = [.width]`. The default (`widthTracksTextView = true`, autoresize on) causes every SwiftUI-driven `setFrameSize` — window resize, eager `VStack` measurement — to forward onto the layout manager, triggering `_fillLayoutHoleForCharacterRange`: a synchronous main-thread relayout that is O(glyph count). Reference: [`NSTextView.isHorizontallyResizable`](https://developer.apple.com/documentation/appkit/nstextview/ishorizontallyresizable).
- **Memoize `sizeThatFits` (and sibling measurement calls) in `NSViewRepresentable` views placed in lazy containers.** Any `NSViewRepresentable` that hosts a TextKit stack (`NSLayoutManager` + `NSTextContainer`) and can appear inside a `LazyVStack` / `LazyHStack` / `List` cell must cache its last measurement. [`NSLayoutManager.ensureLayout(for:)`](https://developer.apple.com/documentation/uikit/nslayoutmanager/ensurelayout(for:)) is O(glyph count) and runs synchronously on the main thread, and SwiftUI calls [`sizeThatFits(_:nsView:context:)`](https://developer.apple.com/documentation/swiftui/nsviewrepresentable/sizethatfits(_:nsview:context:)) on every visible cell on every layout pass (scroll, resize, streaming edit). Cache at minimum on `(textStorage.length, proposalWidth)`; for attribute-sensitive paths (static measurement helpers, highlighted content) key on the `NSAttributedString` itself and fall through to [`NSAttributedString.isEqual(_:)`](https://developer.apple.com/documentation/foundation/nsattributedstring/isequal(_:)) for identity — `NSAttributedString.hash` alone admits collisions and surfaces as wrong-height cells. Invalidate at every text-storage mutation site (text apply, edit callbacks, async highlight/formatting completions). Reference: `SelectableTextView`. When line wrapping is disabled and line height is pinned (as in `VCodeView.VCodeTextView` and `HighlightedTextView.CodeTextView`), prefer the line-count math above over a cached `ensureLayout` call.
<details>
<summary><strong>Per-entity observation and dictionary patterns</strong></summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ struct MarkdownSegmentView: View, Equatable {

VSelectableTextView(
attributedString: measurement.nsAttributedString,
maxWidth: markdownView.maxContentWidth,
maxWidth: measurement.effectiveMaxWidth,
lineSpacing: 4,
tintColor: NSColor(markdownView.tintColor),
useExternalSizing: true
Expand Down Expand Up @@ -410,6 +410,11 @@ struct MarkdownSegmentView: View, Equatable {
let nsAttributedString: NSAttributedString
let size: CGSize
let hasUnresolvedEmphasis: Bool
/// The wrap width used to produce `size`. Callers rendering with
/// `VSelectableTextView(useExternalSizing: true)` must pass this as
/// `maxWidth` so the live text container wraps at the same width
/// that measurement assumed.
let effectiveMaxWidth: CGFloat
}

/// Computes or retrieves from cache the `(NSAttributedString, CGSize)` pair
Expand Down Expand Up @@ -455,7 +460,8 @@ struct MarkdownSegmentView: View, Equatable {
return SelectableRunMeasurementResult(
nsAttributedString: cached.nsAttributedString,
size: cached.size,
hasUnresolvedEmphasis: false
hasUnresolvedEmphasis: false,
effectiveMaxWidth: effectiveMaxWidth
)
}

Expand Down Expand Up @@ -500,7 +506,8 @@ struct MarkdownSegmentView: View, Equatable {
return SelectableRunMeasurementResult(
nsAttributedString: nsAttributed,
size: size,
hasUnresolvedEmphasis: hasUnresolvedEmphasis
hasUnresolvedEmphasis: hasUnresolvedEmphasis,
effectiveMaxWidth: effectiveMaxWidth
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ private final class SelectableNSTextView: NSTextView {
/// view during the layout pass. This avoids an O(N) layout measurement
/// cascade through nested `StackLayout.sizeThatFits` calls.
///
/// **Contract for `useExternalSizing: true`:** `maxWidth` MUST be non-nil
/// and equal to the value passed to `measureSize`. The live text container
/// is sized from `maxWidth`; if it diverges from the measurement width the
/// rendered wrap geometry will not match the precomputed frame, producing
/// horizontal clipping or stale heights.
///
/// For low-instance-count scenarios (e.g., a single thinking block),
/// leave `useExternalSizing` at its default (`false`) and let
/// `sizeThatFits` compute the size normally.
Expand Down Expand Up @@ -204,11 +210,30 @@ public struct VSelectableTextView: NSViewRepresentable {
// VCodeView / HighlightedTextView still opt in to non-contiguous
// layout independently for their own scroll-attachment perf fix.
textStorage.addLayoutManager(layoutManager)

// Two container sizing modes:
//
// - `useExternalSizing: true` — the caller precomputes size via
// `measureSize` and applies `.frame(width:height:)`. The container
// is decoupled from the view frame so that `setFrameSize` cannot
// forward a width change onto the layout manager and trigger
// `_fillLayoutHoleForCharacterRange`, an O(glyph-count) main-thread
// relayout. Container width is sized explicitly from `maxWidth`
// here, and propagated in `updateNSView` when `maxWidth` changes.
//
// - `useExternalSizing: false` — `sizeThatFits` drives layout and
// the container tracks the view frame, matching NSTextView's
// default. Used by the design system gallery only.
//
// Reference: NSTextContainer.widthTracksTextView
// https://developer.apple.com/documentation/uikit/nstextcontainer/widthtrackstextview
let initialContainerWidth = useExternalSizing ? (maxWidth ?? CGFloat.greatestFiniteMagnitude) : 0
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.

🟡 Container width / measurement width mismatch when maxWidth is nil on external-sizing path

When useExternalSizing is true and maxWidth is nil, the text container width is set to CGFloat.greatestFiniteMagnitude (line 229: maxWidth ?? CGFloat.greatestFiniteMagnitude), but the measurement in resolveSelectableRunMeasurementResult (clients/macos/vellum-assistant/Features/Chat/MarkdownSegmentView.swift:438) uses effectiveMaxWidth = maxContentWidth ?? VSpacing.chatBubbleMaxWidth. Before this PR, widthTracksTextView was always true, so the container followed the view frame (set by the caller's .frame(width:height:) from the measurement), keeping wrapping consistent. Now, with widthTracksTextView = false on the external-sizing path, the container stays at greatestFiniteMagnitude — text won't wrap at the measured width, causing a rendering/measurement mismatch (text clipped horizontally, incorrect height). This affects SubagentDetailPanel (clients/macos/vellum-assistant/Features/MainWindow/Panels/SubagentDetailPanel.swift:203-205) during its initial layout pass when panelContentWidth == 0markdownWidth = nil. The issue self-corrects when onGeometryChange fires a valid width.

Prompt for agents
The root cause is the nil fallback to CGFloat.greatestFiniteMagnitude for the container width on the external-sizing path. The measurement path in resolveSelectableRunMeasurementResult (MarkdownSegmentView.swift:438) falls back to VSpacing.chatBubbleMaxWidth when maxContentWidth is nil, but the container fallback uses greatestFiniteMagnitude. These two fallbacks need to agree.

Two possible approaches:
1. In SelectableTextView.swift, change the nil fallback for container width to match the measurement fallback. However, VSelectableTextView does not (and should not) know about VSpacing.chatBubbleMaxWidth.
2. In MarkdownSegmentView.SelectableRunView (line 197), pass the effectiveMaxWidth (maxContentWidth ?? VSpacing.chatBubbleMaxWidth) instead of the raw maxContentWidth to VSelectableTextView's maxWidth parameter. This ensures the container always gets the same concrete width that was used for measurement.

Approach 2 is cleaner because it keeps the contract between measurement and container explicit at the call site.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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.

Fixed in 843fa36 — same approach as the codex thread: SelectableRunMeasurementResult now exposes effectiveMaxWidth, and the useExternalSizing: true call site passes it directly so live container width and measurement width can't drift. The SubagentDetailPanel initial-layout case (panelContentWidth == 0markdownWidth = nil) is covered because MarkdownSegmentView.maxContentWidth defaults to VSpacing.chatBubbleMaxWidth and the measurement resolver applies the same fallback.

let textContainer = NSTextContainer(size: NSSize(
width: 0,
width: initialContainerWidth,
Comment thread
ashleeradka marked this conversation as resolved.
height: CGFloat.greatestFiniteMagnitude
))
textContainer.widthTracksTextView = true
textContainer.widthTracksTextView = !useExternalSizing
textContainer.heightTracksTextView = false
textContainer.lineFragmentPadding = 0
layoutManager.addTextContainer(textContainer)

Expand All @@ -220,8 +245,22 @@ public struct VSelectableTextView: NSViewRepresentable {
textView.backgroundColor = .clear
textView.drawsBackground = false
textView.isVerticallyResizable = true
textView.isHorizontallyResizable = false
textView.autoresizingMask = [.width]
// Allow the text view to grow independently of its frame when
// `useExternalSizing` is true. This reinforces the container
// decoupling above: if `setFrameSize` is invoked with a width
// smaller than the current content, AppKit will not shrink the
// container and trigger a re-layout of all glyphs.
textView.isHorizontallyResizable = useExternalSizing
if useExternalSizing {
textView.maxSize = NSSize(
width: CGFloat.greatestFiniteMagnitude,
height: CGFloat.greatestFiniteMagnitude
)
} else {
// Match NSTextView's default autoresizing so the container
// width tracks the view frame in the Gallery preview path.
textView.autoresizingMask = [.width]
}
Comment on lines +253 to +263
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.

🚩 isHorizontallyResizable = true with maxSize may cause unexpected text view behavior

Setting isHorizontallyResizable = true (SelectableTextView.swift:253) combined with maxSize = (.greatestFiniteMagnitude, .greatestFiniteMagnitude) (lines 255-258) allows the NSTextView to grow beyond its frame. This is intentional — it prevents setFrameSize (triggered by SwiftUI layout) from forwarding width changes onto the layout manager and causing O(glyph-count) relayouts. Since widthTracksTextView = false and the container is sized explicitly, the text view's frame can diverge from the container without affecting rendering. The .frame(width:height:) applied by SelectableRunView constrains the visible area. Worth monitoring if any AppKit drawing artifacts appear with this configuration.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

textView.textContainerInset = .zero

textView.delegate = context.coordinator
Expand All @@ -232,12 +271,29 @@ public struct VSelectableTextView: NSViewRepresentable {
.cursor: NSCursor.pointingHand,
]

context.coordinator.lastContainerWidth = useExternalSizing ? maxWidth : nil
context.coordinator.applyAttributedString(attributedString, lineSpacing: lineSpacing, to: textView)
return textView
}

public func updateNSView(_ textView: NSTextView, context: Context) {
let coordinator = context.coordinator

// Propagate container width changes before applying any text update.
// On the external-sizing path, the container is decoupled from the
// view frame, so a new `maxWidth` (e.g. from a window resize) must
// be written onto the container explicitly. Skip when unchanged so
// we do not perturb the current layout.
if useExternalSizing,
let textContainer = textView.textContainer,
coordinator.lastContainerWidth != maxWidth {
textContainer.containerSize = NSSize(
width: maxWidth ?? CGFloat.greatestFiniteMagnitude,
height: CGFloat.greatestFiniteMagnitude
)
coordinator.lastContainerWidth = maxWidth
}

guard coordinator.lastAttributedString != attributedString
|| coordinator.lastLineSpacing != lineSpacing else { return }
if useExternalSizing {
Expand Down Expand Up @@ -326,13 +382,20 @@ public struct VSelectableTextView: NSViewRepresentable {
var lastMeasuredWidth: CGFloat = -1
var lastMeasuredSize: CGSize = .zero

// Width currently written onto the text container on the external-
// sizing path. `updateNSView` propagates changes when this drifts
// from the incoming `maxWidth`, so the container stays decoupled
// from the view frame without re-sizing on every SwiftUI update.
var lastContainerWidth: CGFloat?

func reset() {
lastAttributedString = nil
lastLineSpacing = 0
pendingAttributedString = nil
pendingLineSpacing = nil
pendingTextView = nil
hasScheduledApply = false
lastContainerWidth = nil
invalidateMeasurementCache()
}

Expand Down