perf: eliminate FlexFrameLayout anti-patterns causing 2s+ LazyVStack hangs#25844
Conversation
…hangs Replace _FlexFrameLayout patterns with _FrameLayout (definite dimensions) in two files to stop alignment cascade and recursive measurement inside LazyVStack cells. InlineFilePreviewView: - Replace ScrollView.frame(maxHeight: 400) with two-path pattern: definite- height ScrollView for long content (>25 lines or >50KB), no ScrollView for short content - Replace .frame(maxWidth: .infinity, alignment: .center) in loadingView and errorView with HStack+Spacer centering - Fix loadError not being reset on successful content load - Extract markdownContent into a shared computed property ChatBubbleAttachmentContent: - Replace .frame(maxWidth:, maxHeight:) with computed definite .frame(width:, height:) using aspect-fit calculation in InlineToolCallImageView.imageContent and AttachmentImageGrid.singleImageContent - Replace .frame(maxWidth:) fallback with .frame(width:) for images without CGImage representation - Replace two-frame placeholder pattern with single definite .frame(width:, height:) for single-image placeholders - Guard against nativeWidth == 0 to prevent division by zero Fixes LUM-835 Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
🤖 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:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b23f8af2d8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| maxWidth: min(nativeWidth, maxDim), | ||
| maxHeight: min(nativeHeight, maxDim) | ||
| ) | ||
| .frame(width: fitWidth, height: fitHeight) |
There was a problem hiding this comment.
Using a fixed frame(width: fitWidth, height: fitHeight) here removes the parent-driven shrinking behavior that maxWidth previously provided. In narrow chat layouts (where the transcript computes a smaller bubble width than VSpacing.chatBubbleMaxWidth), single-image attachments can now render wider than their bubble and appear clipped/overflowed. This should be clamped to the effective bubble width (for example via the bubbleMaxWidth environment value) before applying a fixed frame.
Useful? React with 👍 / 👎.
| let lineCount = content.utf8.reduce(1) { $0 + ($1 == 0x0A ? 1 : 0) } | ||
| if lineCount > Self.lineThreshold || content.utf8.count > Self.charThreshold { |
There was a problem hiding this comment.
This computes newline and byte counts directly inside contentArea, so every SwiftUI body re-evaluation rescans the full file text. Expanded previews can re-render frequently while chat state updates, and large attachments will pay repeated O(n) UTF-8 scans on the main thread, causing avoidable jank. These thresholds should be computed once when cachedContent changes and stored in state.
Useful? React with 👍 / 👎.
ashleeradka
left a comment
ashleeradka
left a comment
There was a problem hiding this comment.
Approve with one fix needed.
InlineFilePreviewView changes are clean — two-path pattern with extracted markdownContent, HStack+Spacer centering, loadError reset on success, well-documented thresholds. The cgImage path in ChatBubbleAttachmentContent is correct with the aspect-fit scale-clamping math.
Issue: NSImage fallback paths (lines ~144 and ~385) upscale small images.
The old code used .frame(maxWidth: chatBubbleMaxWidth) which let small images stay at their natural size. The new .frame(width: chatBubbleMaxWidth) forces ALL fallback images to exactly 760pt wide. With .resizable().aspectRatio(contentMode: .fit), a small icon or thumbnail gets upscaled to fill 760pt — blurry and wrong.
The cgImage path correctly prevents upscaling with min(..., 1.0) in the scale factor, but the fallback paths don't have equivalent protection.
Fix: Replace .frame(width: VSpacing.chatBubbleMaxWidth) in the fallback paths with the HStack+Spacer pattern:
HStack(spacing: 0) {
Image(nsImage: renderImage)
.resizable()
.aspectRatio(contentMode: .fit)
.frame(maxWidth: VSpacing.chatBubbleMaxWidth) // keep maxWidth here — no cgImage dims to compute definite size
.clipShape(RoundedRectangle(cornerRadius: VRadius.md))
Spacer(minLength: 0)
}This avoids the FlexFrame alignment cascade (HStack+Spacer handles leading alignment without triggering explicitAlignment queries on children) while preserving the natural sizing behavior for small images.
— Reviewed by Vex ✦
…age upscaling Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
|
Re: NSImage fallback paths upscaling small images (@ashleeradka) Fixed in 4923c43. Both fallback paths now use |
|
Re: Codex P1 — Clamp image frame width to available bubble width The cgImage path now uses definite Re: Codex P2 — Avoid rescanning attachment text on every view recompute
|
Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
HStack+Spacer does NOT fully block the _FlexFrameLayout alignment cascade — Apple's Layout protocol default explicitAlignment merges subviews' guides, recursing into children. Use NSImage.size to compute definite frame(width:height:) instead, matching the cgImage path and producing _FrameLayout (O(1), no alignment queries). Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
| if lineCount > Self.lineThreshold || content.utf8.count > Self.charThreshold { | ||
| ScrollView { | ||
| markdownContent | ||
| } | ||
| .frame(height: Self.maxContentHeight) | ||
| } else { | ||
| markdownContent | ||
| } |
There was a problem hiding this comment.
🚩 Nested vertical ScrollViews for long non-markdown files
For non-markdown files exceeding 25 lines, InlineFilePreviewView wraps markdownContent in a ScrollView with height: 400 (InlineFilePreviewView.swift:146-149). The markdownContent renders a MarkdownSegmentView which produces a CodeBlockView. CodeBlockView has its own vertical+horizontal ScrollView with height: 400 for long code (MarkdownSegmentView.swift:889-897). This creates nested vertical ScrollViews with the same height cap. The outer ScrollView is slightly scrollable (inner is 400pt + header + padding > 400pt), which is a suboptimal UX. However, this is a pre-existing issue — the old code always wrapped in ScrollView { ... }.frame(maxHeight: 400), producing the same nesting. The new code is actually better for medium-length content (23-25 lines) where the outer ScrollView is now omitted entirely.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed — this is a pre-existing issue as the review notes. The old code always wrapped in ScrollView { }.frame(maxHeight: 400), producing the same nesting. This PR improves the situation for medium-length content by omitting the outer ScrollView entirely. The nested ScrollView case for long non-markdown files could be addressed in a follow-up but is orthogonal to the FlexFrame fixes here.
ashleeradka
left a comment
ashleeradka
left a comment
There was a problem hiding this comment.
All feedback addressed. NSImage fallback now uses renderImage.size (point dimensions) with the same scale-clamping math as the cgImage path — no upscaling, correct aspect ratio, zero-dimension guard. Placeholder width change is fine since it's a transient loading state.
InlineFilePreviewView two-path pattern is clean. AGENTS.md rule addition is good for regression prevention.
Ready for Boss QA. Test plan:
- File preview with short content (<25 lines) — should render at natural height, no ScrollView
- File preview with long content (>25 lines) — should cap at 400pt with scrolling
- Images: small images should stay at natural size (no upscaling), large images should scale down to fit 760pt
- Image placeholders during loading — should show correctly
- File preview retry after error — should clear error state on success
— Vex ✦
…rendering Extract the duplicated aspect-fit sizing logic and image rendering pattern from InlineToolCallImageView.imageContent and AttachmentImageGrid.singleImageContent into a reusable AspectFitImageView struct. The new view encapsulates: - CGImage pixel → point conversion (divided by displayScale) - NSImage.size fallback for point dimensions - Aspect-fit scale computation with min(..., 1.0) upscale clamp - Definite frame(width:height:) sizing (FrameLayout, not FlexFrameLayout) - Rounded corner clipping Both call sites now use a single AspectFitImageView initializer instead of 30+ lines of duplicated logic each. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
ashleeradka
deleted the
devin/LUM-835-1776269239-fix-flexframe-antipatterns
branch
|
🖥️ macOS app artifact for this PR is ready for download: Download vellum-assistant-pr-25844.dmg To run this build locally: |
…ssions (#27554) Adds a fast ripgrep-based guard that fails CI when a new `.frame(maxWidth:)` or `.frame(maxHeight:)` is introduced inside `Features/Chat/` or `Features/MainWindow/`. These modifiers create `_FlexFrameLayout`, which cascades `explicitAlignment` queries through descendants and has caused multi-second hangs in LazyVStack-backed chat surfaces 9+ times (PRs #24019, #24091, #24584, #24589, #25844, #25947, #26007, #26053, #26092, #26220). The manual audit process missed regressions twice — this lint enforces the AGENTS.md:277-286 rule mechanically. Tracked in LUM-1116. Content-hash allowlist (`clients/scripts/flexframe-allowlist.txt`) seeded with the 170 existing occurrences so the check passes on current main. Entries are keyed on `<path>|<trimmed-line>` so unrelated line drift doesn't break them.
… (LUM-1117) Chat images, inline document previews, and thinking blocks all computed their width from the static VSpacing.chatBubbleMaxWidth (760pt) and fed it into a definite .frame(width:). At non-fullscreen window widths the content stayed 760pt wide and overflowed the actual chat column. ChatBubble already threads a container-aware bubbleMaxWidth via Environment, but these peer views never read it. Swap the static reference for an @Environment(\.bubbleMaxWidth) read in each site, falling back to the 760 default only when the env is 0 (first layout pass, per MessageListLayoutMetrics) or unset (non-chat contexts). Sites: - AnimatedImageView — markdown inline images. - InlineToolCallImageView — tool-generated images in the message flow. - AttachmentImageGrid — single-image attachments and pre-decode placeholder. Decode targetSize kept static so window resize doesn't re-fire the decode task on every frame. - InlineFilePreviewView — markdown budget for the expanded document card. - ThinkingBlockView — same pattern, inherited the same static cap. Shape stays .frame(width: computed) throughout; no .frame(maxWidth:) is introduced, so the _FlexFrameLayout hang patterns fixed in #25844, #26053, and #26092 stay out. Adds a durable AGENTS.md bullet describing the env-vs-token contract so the next editor doesn't reach for the static token by default. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… (LUM-1117) (#27557) Chat images, inline document previews, and thinking blocks all computed their width from the static VSpacing.chatBubbleMaxWidth (760pt) and fed it into a definite .frame(width:). At non-fullscreen window widths the content stayed 760pt wide and overflowed the actual chat column. ChatBubble already threads a container-aware bubbleMaxWidth via Environment, but these peer views never read it. Swap the static reference for an @Environment(\.bubbleMaxWidth) read in each site, falling back to the 760 default only when the env is 0 (first layout pass, per MessageListLayoutMetrics) or unset (non-chat contexts). Sites: - AnimatedImageView — markdown inline images. - InlineToolCallImageView — tool-generated images in the message flow. - AttachmentImageGrid — single-image attachments and pre-decode placeholder. Decode targetSize kept static so window resize doesn't re-fire the decode task on every frame. - InlineFilePreviewView — markdown budget for the expanded document card. - ThinkingBlockView — same pattern, inherited the same static cap. Shape stays .frame(width: computed) throughout; no .frame(maxWidth:) is introduced, so the _FlexFrameLayout hang patterns fixed in #25844, #26053, and #26092 stay out. Adds a durable AGENTS.md bullet describing the env-vs-token contract so the next editor doesn't reach for the static token by default. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Replaces
_FlexFrameLayoutpatterns (.frame(maxWidth:),.frame(maxHeight:)) with definite-dimension_FrameLayout(.frame(width:, height:)) andHStack+Spacercentering inside LazyVStack cell views, eliminating the recursiveexplicitAlignmentcascade that causes 2s+ main-thread hangs (Sentry: VELLUM-ASSISTANT-MACOS-9B).Why needed
.frame(maxWidth:)/.frame(maxHeight:)compile to_FlexFrameLayout, whose default.centeralignment triggersexplicitAlignmentqueries that cascade O(n × depth) through every LazyVStack cell subtree..frame(width:, height:)compiles to_FrameLayout, returningbounds.midXwithout querying children — O(1).Confirmed via A/B spin dumps: main branch shows
LazySubviewPrefetcher → sizeThatFits → explicitAlignmentcascade (8+ levels deep through nested_HStackLayout); fix branch shows zero samples in this path.Changes
ScrollView { }.frame(maxHeight: 400)→ two-path pattern: content exceeding 25 lines or 50 KB getsScrollView { }.frame(height: 400), short content renders directly without a ScrollView.loadingView/errorViewcentering changed from.frame(maxWidth: .infinity, alignment: .center)toHStack + Spacer. Also resetsloadErroron successful content load (bug fix).AspectFitImageView, a reusable struct that computes aspect-fit dimensions viacomputeFitSize(CGImage pixels ÷ displayScale, falling back toNSImage.size) and renders with definiteframe(width:height:). Replaces ~60 lines of duplicated logic acrossInlineToolCallImageViewandAttachmentImageGrid.singleImageContent. Placeholder rectangle collapsed from two-frame to single definiteframe(width:, height:).frame(maxHeight:)ScrollView anti-pattern and two-path alternative.Why safe
Same pattern proven across 5+ cell views in PR #24091. The
computeFitSizeformula produces identical bounding boxes to the oldmin(nativeWidth, maxDim)/min(nativeHeight, maxDim)with.fit. Themin(..., 1.0)scale clamp prevents upscaling small images.Alternatives not taken
onGeometryChange+@StateheightGeometryReader.containerRelativeFrameHStack+Spacerwrapping inner.frame(maxWidth:)Layout.explicitAlignmentdocs confirm default impl "merges the subviews' guides" — HStack recurses into children, does NOT block the cascadeReferences
Prompt / plan
Investigated LUM-835 by tracing Sentry stacktraces (9B, GJ, EP) to identify three hang mechanisms in LazyVStack cells. Fixed by replacing
_FlexFrameLayoutwith_FrameLayoutbarriers at the cell level and extracting duplicated aspect-fit logic intoAspectFitImageView.Test plan
AspectFitImageViewcallscgImage(forProposedRect:)twice (once incomputeFitSize, once inbody). NSImage caches internally — verify no visible perf regression with many images.width: VSpacing.chatBubbleMaxWidth— verify correct when bubble container is narrower.contentAreabranching relies oncachedContentbeing set-once — if this invariant changes, theif/elsecould cause view identity thrash.Link to Devin session: https://app.devin.ai/sessions/43a199c477c3412b8774f48c90ff341f
Requested by: @ashleeradka