fix(macos): decouple VSelectableTextView text container from view frame#28617
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // and the container tracks the view frame as NSTextView's | ||
| // default behavior intends. Used by the Gallery preview, where | ||
| // there is only a single instance and no resize storm. | ||
| let initialContainerWidth = useExternalSizing ? (maxWidth ?? CGFloat.greatestFiniteMagnitude) : 0 |
There was a problem hiding this comment.
🟡 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 == 0 → markdownWidth = 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.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
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 == 0 → markdownWidth = nil) is covered because MarkdownSegmentView.maxContentWidth defaults to VSpacing.chatBubbleMaxWidth and the measurement resolver applies the same fallback.
…rnal-sizing path (LUM-1170) Closes LUM-1170 Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
…SelectableTextView comments Closes LUM-1170 Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
devin-ai-integration
Bot
force-pushed
the
devin/1777386256-lum-1170-selectable-textview-container
branch
from
952418f to
774dfed
Compare
devin-ai-integration
Bot
changed the title
…TextView Both reviewers flagged that VSelectableTextView's container width fell back to .greatestFiniteMagnitude on the useExternalSizing path when maxWidth was nil, while the measurement path fell back to VSpacing.chatBubbleMaxWidth. The two fallbacks must agree or the live container will not wrap at the measured width, producing horizontal clipping until a real width arrives. Carry the resolved effectiveMaxWidth on SelectableRunMeasurementResult and pass it directly to VSelectableTextView so the call site cannot drift from the measurement value. Document the contract on VSelectableTextView so future external-sizing callers do the same. Closes LUM-1170 Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
✦ APPROVE
Value: Eliminates the O(glyph-count) _fillLayoutHoleForCharacterRange main-thread hang in VSelectableTextView during window resize and eager VStack measurement by decoupling the NSTextContainer width from the NSTextView frame on the useExternalSizing path.
What this does: When useExternalSizing: true, the container is now sized explicitly from maxWidth at init time (widthTracksTextView = false, isHorizontallyResizable = true). setFrameSize can no longer forward a width change onto the layout manager. updateNSView propagates maxWidth changes onto the container directly. A new effectiveMaxWidth field on SelectableRunMeasurementResult ensures the single call site passes the same resolved width to both measurement and the live container, eliminating the nil-width divergence flagged by Codex and Devin.
Root cause fix is correct. The pre-PR behavior (widthTracksTextView = true + autoresizingMask = [.width]) caused every SwiftUI-driven setFrameSize — window resize, eager VStack measurement from message-height-cache flag ON — to call NSTextView._setFrameSize:forceScroll: → _glyphRangeForBoundingRect → _fillLayoutHoleForCharacterRange. The fix matches the VCodeView/HighlightedTextView pattern and is exactly what Devin's investigation identified.
The Codex/Devin maxWidth nil concern is resolved. The original PR had makeNSView initialize container width to CGFloat.greatestFiniteMagnitude when maxWidth is nil, while measurement used effectiveMaxWidth = maxContentWidth ?? VSpacing.chatBubbleMaxWidth. The fix introduces SelectableRunMeasurementResult.effectiveMaxWidth and SelectableRunView now passes measurement.effectiveMaxWidth directly to VSelectableTextView.maxWidth, making live container and measurement always agree. Verified on the PR branch.
useExternalSizing: false path is untouched. Gallery-mode behavior (widthTracksTextView = true, autoresizingMask = [.width], sizeThatFits drives layout) is preserved exactly. The only call site with useExternalSizing: true is SelectableRunView in MarkdownSegmentView.
AGENTS.md guidance added. New entry correctly documents the pattern contract for future callers of useExternalSizing: true. Good hygiene.
One non-blocking observation: makeNSView still falls back to CGFloat.greatestFiniteMagnitude if maxWidth is nil on the external-sizing path (line let initialContainerWidth = useExternalSizing ? (maxWidth ?? CGFloat.greatestFiniteMagnitude) : 0). With the effectiveMaxWidth fix this nil case can't be reached from the current call site, but the defensive fallback in updateNSView uses the same maxWidth ?? CGFloat.greatestFiniteMagnitude, keeping behavior consistent. The contract doc on VSelectableTextView now explicitly states maxWidth MUST be non-nil on the external-sizing path, which is the right enforcement layer.
Merge criteria check:
- ✅ Root cause matches MACOS-D9 / LUM-1170 Devin investigation findings
- ✅ Bot concern (maxWidth nil mismatch) resolved by
effectiveMaxWidth - ✅ AGENTS.md updated with pattern contract
- ✅ Only one
useExternalSizing: truecall site, fully updated ⚠️ CI macOS build/tests skipped — needs a clean build confirmation before merge⚠️ Devin and Codex both COMMENTED, not APPROVED (their findings are addressed but reviews not re-triggered)
Suggest: trigger @devin review this PR + @codex review to get formal approvals on the updated commit, then merge.
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
| 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] | ||
| } |
There was a problem hiding this comment.
🚩 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.
Was this helpful? React with 👍 or 👎 to provide feedback.
ashleeradka
deleted the
devin/1777386256-lum-1170-selectable-textview-container
branch
|
Devin is archived and cannot be woken up. Please unarchive Devin if you want to continue using it. |
1 participant
VSelectableTextViewpreviously letNSTextContainer.widthTracksTextView = trueandautoresizingMask = [.width]couple the container width to theNSTextViewframe. On theuseExternalSizing: truepath — where SwiftUI applies a.frame(width:height:)precomputed by the staticmeasureSizehelper — every SwiftUI-drivensetFrameSize(window resize, eagerVStackmeasurement) forwarded onto the layout manager and triggered_fillLayoutHoleForCharacterRange: a synchronous, O(glyph-count) main-thread relayout. This change decouples the container from the frame on that path and alignsVSelectableTextViewwith the established peers (VCodeView,HighlightedTextView).Why this is needed
The
useExternalSizing: truecallers (every chat message inMarkdownSegmentView) hand SwiftUI an exact frame derived frommeasureSize(...). AppKit's response tosetFrameSizeonly mattered to legacy auto-layout flows, and on this path it is purely harmful: it re-runs glyph layout on the entire message every time SwiftUI resizes the host. With many message rows visible (eagerVStack, window resize), the work compounds into multi-second main-thread hangs.Benefits
setFrameSize→_fillLayoutHoleForCharacterRangepath observed under load.VSelectableTextViewin line with peerNSTextViewbridges in the design system (VCodeView,HighlightedTextView), which already configure their containers this way; one less divergent pattern acrossclients/shared/DesignSystem.VSelectableTextView(window resize, split-view drag, layout cascade), the container no longer gets dragged along.Why this is safe
useExternalSizingcontract already provides.frame(width:height:)frommeasureSize(...), which itself uses anNSTextContainerof(maxWidth, .greatestFiniteMagnitude). Sizing the live container with the same dimensions reproduces the same wrap geometry the measurement assumed, so visible layout is unchanged.updateNSViewpropagatesmaxWidthchanges ontocontainerSizeso window-resize and split-view-drag still re-wrap text correctly.useExternalSizing: falsebranch (used only by the design system gallery) is byte-for-byte the previous configuration; it still callssizeThatFitsand lets the container track the frame.useExternalSizingis unchanged.References
NSTextContainer.widthTracksTextViewNSTextContainer.containerSizeNSTextView.isHorizontallyResizableNSLayoutManager— Performance IssuesAlternatives not taken
allowsNonContiguousLayouton theVSelectableTextViewlayout manager. Rejected. The render stack here is paired with a sibling measurement stack that lays out every glyph; non-contiguous render layout produces streaming gap/overlap (text paints into a smaller region than the measured frame, then the next sibling is positioned via the measured frame and the lazy glyphs paint outside it).clients/AGENTS.mdalready documents this exception andVCodeView/HighlightedTextViewonly opt into non-contiguous layout because they don't share the same measurement model.message-height-cacheflag fix (LUM-1167). Insufficient. That change removes the eager-measurement trigger but leaves the frame-coupling vulnerability in place — any future caller that resizes aVSelectableTextViewframe would re-introduce the same hang. Fixing the underlying configuration is the durable repair.widthTracksTextViewunconditionally (dropuseExternalSizingentirely). Rejected for now to keep the change scoped. The gallery preview path still relies onsizeThatFits-driven layout and the existing tracking behavior. Consolidating to a single mode is reasonable follow-up work but is not blocking this fix.Root cause analysis
widthTracksTextView = true,autoresizingMask = [.width]) — the configuration AppKit recommends when the text view is the source of truth for sizing.useExternalSizingwas added later as a perf optimization to skipsizeThatFitsin lazy containers, but the AppKit-default configuration inmakeNSViewwas not revisited to match the new contract (caller-supplied frame viameasureSize).nilfromsizeThatFitswas sufficient to bypass measurement work. It missed thatsetFrameSizeitself — which SwiftUI invokes when applying.frame(width:height:)— has its own AppKit-internal layout cost when the container is configured to track the frame.VCodeView,HighlightedTextView) had already converged onwidthTracksTextView = falseplus an explicitcontainerSize. The divergence was visible but never flagged as a perf concern because the symptom only surfaces under combined eager-measurement + large-text load. Sentry MACOS-D9 grouped this hang with the older LUM-712 path, masking that a second, distinct bug existed.NSTextView-bridging design system components on the same configuration. The procedural fix is the AGENTS.md addition below — a checklist item that makes this configuration explicit for futureNSViewRepresentableauthors.clients/AGENTS.md§ "SwiftUI performance practices" documenting the rule with Apple references. Kept it lightweight per the existing tone (rule + links, no PR-specific narrative). The rule is durable: it describes a configuration invariant, not a transient migration.Prompt / plan
LUM-1170. Investigated the v0.6.5 Sentry hang on
NSTextView.setFrameSize→_fillLayoutHoleForCharacterRange, identified the frame/container coupling on theuseExternalSizingpath, and aligned the configuration with the peerNSTextViewbridges in the design system.Test plan
VCodeView/HighlightedTextViewprovides reference for visual correctness.Closes LUM-1170
Link to Devin session: https://app.devin.ai/sessions/b98a97a718fe4330a8ef08ddf3ba8645
Requested by: @ashleeradka