Fix ScrollView.frame(maxHeight:) anti-pattern in 5 LazyVStack cell files#24091
Conversation
…remaining LazyVStack cells Replace _FlexFrameLayout (recursive measurement, O(n × depth)) with _FixedSizeLayout (O(1)) or no ScrollView for short content. - CodePreviewView: two-path — >7 lines: ScrollView.frame(height: 120), otherwise plain Text - InlineChatErrorAlert: two-path — >10 lines: ScrollView.frame(height: 160), otherwise plain Text - GuardianDecisionBubble: two-path — >7 lines: ScrollView.frame(height: 120), otherwise plain Text - VDiffView: frame(maxHeight:) → frame(height:) — callers pass definite values Line counting uses zero-allocation utf8.reduce byte scan. 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:
|
When maxHeight is provided but content is short (lines.count <= maxHeight/16), use the horizontal-only scroll with fixedSize instead of forcing a definite height. Prevents empty space below short diffs in ToolConfirmationBubble (260pt) and Gallery (120pt). Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
Catches mega-strings (e.g. minified JSON, base64 data) that are a single line but exceed 50,000 UTF-8 bytes. Consistent with adaptiveScrollFrame's charThreshold pattern from PR #24019. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
ashleeradka
left a comment
There was a problem hiding this comment.
✦ Vex review: Ship it.
All 4 files correctly implement the two-path pattern from #24019:
- CodePreviewView — 7-line threshold, 120pt definite height, utf8.reduce + 50k mega-string guard ✅
- GuardianDecisionBubble — 7 lines / 120pt, textSelection preserved on both paths ✅
- InlineChatErrorAlert — 10 lines / 160pt, outer padding unchanged ✅
- VDiffView — dynamic threshold via
Int(maxHeight / 16), short path preserves natural height with.fixedSize(horizontal: false, vertical: true)✅
Minor note: displayCode is a computed property, so it's evaluated twice (once for lineCount, once for Text). Acknowledged in the PR body as negligible for ≤30 lines — agreed.
Ready for local build + smoke test.
Additional Fix Required: CodeBlockView in MarkdownSegmentView.swiftThe 4 files fixed in this PR are correct ✅, but QA testing revealed the biggest offender is missing: the File
Current Code (problematic)let codeLineCount = code.components(separatedBy: "\n").count
let codeBlockHeight = CGFloat(codeLineCount) * Self.codeLineHeight + VSpacing.sm * 2
ScrollView(.horizontal, showsIndicators: false) {
Text(code)
.font(.custom("DMMono-Regular", size: 13))
.foregroundStyle(textColor)
.textSelection(.enabled)
.fixedSize(horizontal: true, vertical: true)
.padding(VSpacing.sm)
}
.frame(height: codeBlockHeight)Problem
Fix DirectionApply the same two-path pattern used in the other 4 files: private static let maxCodeBlockHeight: CGFloat = 400
private static let lineThreshold = 25 // ~25 lines * 16pt ≈ 400pt
// ...inside body...
let codeLineCount = code.utf8.reduce(0) { $0 + ($1 == UInt8(ascii: "\n") ? 1 : 0) } + 1
if codeLineCount > Self.lineThreshold {
// Long code block: both horizontal AND vertical scroll, capped height
ScrollView([.horizontal, .vertical], showsIndicators: true) {
Text(code)
.font(.custom("DMMono-Regular", size: 13))
.foregroundStyle(textColor)
.textSelection(.enabled)
.fixedSize(horizontal: true, vertical: true)
.padding(VSpacing.sm)
}
.frame(height: Self.maxCodeBlockHeight) // definite height, O(1) layout
} else {
// Short code block: horizontal scroll only, natural height
let codeBlockHeight = CGFloat(codeLineCount) * Self.codeLineHeight + VSpacing.sm * 2
ScrollView(.horizontal, showsIndicators: false) {
Text(code)
.font(.custom("DMMono-Regular", size: 13))
.foregroundStyle(textColor)
.textSelection(.enabled)
.fixedSize(horizontal: true, vertical: true)
.padding(VSpacing.sm)
}
.frame(height: codeBlockHeight)
}Key Rules
This is the component causing the 53-second hang in the spindump analysis. The other 4 files were correct but this was the main culprit for chat message rendering. |
…yVStack cells CodeBlockView previously grew linearly with no cap — a 500-line code block produced ~8000pt height, forcing enormous cell measurement inside LazyVStack. Two-path pattern: - Long code (>25 lines or single-line >50k UTF-8 bytes): both horizontal and vertical scroll axes with .frame(height: 400) — definite _FixedSizeLayout, O(1) - Short code: horizontal-only scroll at natural height (existing behavior) Also replaces .components(separatedBy:) with zero-allocation utf8.reduce byte-scan for line counting, consistent with the other 4 files in this PR. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
|
Fixed the CodeBlockView in 2563c5d. Now uses the two-path pattern:
Also replaced |
ashleeradka
left a comment
There was a problem hiding this comment.
Re-reviewed after CodeBlockView addition. All 5 files now correctly implement the two-path pattern:
CodeBlockView (MarkdownSegmentView.swift) — the big one:
- lineThreshold=25 (derived from 400pt / codeLineHeight), maxCodeBlockHeight=400
- Long path:
ScrollView([.horizontal, .vertical]).frame(height: 400)— definite height, O(1) via_FixedSizeLayout✅ - Short path: horizontal-only
ScrollView.frame(height: codeBlockHeight)— still definite (lineCount * lineHeight + padding) ✅ - Line counting switched from
.components(separatedBy:)toutf8.reducebyte-scan ✅ - 50k mega-string guard on single-line ✅
CodePreviewView — 7 lines / 120pt ✅
GuardianDecisionBubble — 7 lines / 120pt ✅
InlineChatErrorAlert — 10 lines / 160pt ✅
VDiffView — dynamic Int(maxHeight/16) threshold, short path .fixedSize(horizontal: false, vertical: true) ✅
No .frame(maxHeight:) remains in any ScrollView. Ship it.
…ockView threshold from font metrics VDiffView: changed divisor from 16 to 18 to avoid boundary-case overshoot where short-path content could slightly exceed maxHeight (~12pt at 260pt). CodeBlockView: lineThreshold is now computed as Int(maxCodeBlockHeight / codeLineHeight) instead of hardcoded 25, so it stays correct if the mono font changes. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
|
|
||
| /// Line threshold derived from maxCodeBlockHeight / codeLineHeight. | ||
| /// Content above this count takes the capped-height ScrollView path. | ||
| private static let lineThreshold: Int = Int(maxCodeBlockHeight / codeLineHeight) |
There was a problem hiding this comment.
🟡 CodeBlockView lineThreshold ignores padding, causing short-path height to exceed maxCodeBlockHeight
The lineThreshold at MarkdownSegmentView.swift:788 is computed as Int(maxCodeBlockHeight / codeLineHeight) but doesn't account for the VSpacing.sm * 2 (16pt) padding added in the short-path height formula at line 835: CGFloat(codeLineCount) * Self.codeLineHeight + VSpacing.sm * 2. For code blocks at exactly lineThreshold lines, the short-path height is lineThreshold * codeLineHeight + 16, which exceeds maxCodeBlockHeight (400pt) by up to 16pt. For example, if codeLineHeight = 16pt, then lineThreshold = 25 and a 25-line block renders at 416pt (short path), while a 26-line block renders at 400pt (long path) — a visual discontinuity where adding one line makes the view shorter.
| private static let lineThreshold: Int = Int(maxCodeBlockHeight / codeLineHeight) | |
| private static let lineThreshold: Int = Int((maxCodeBlockHeight - VSpacing.sm * 2) / codeLineHeight) |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Devin is archived and cannot be woken up. Please unarchive Devin if you want to continue using it. |
…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.
Summary
Eliminates all remaining
.frame(maxHeight:)on ScrollViews inside LazyVStack cells — the anti-pattern that caused the 30–50 second main-thread hang fixed in #24019..frame(maxHeight:)compiles to_FlexFrameLayout(recursive child measurement, O(n × depth));.frame(height:)compiles to_FixedSizeLayout(O(1), never measures children).All 5 files use a two-path pattern: long content gets a definite-height ScrollView; short content gets no ScrollView or natural-height rendering. A
charThresholdguard (50k UTF-8 bytes) catches single-line mega-strings that would wrap into many visual lines.MarkdownSegmentView.swift).components(separatedBy:)withutf8.reduce.ChatWidgetViews.swift)Text, no ScrollViewText, no ScrollViewText, no ScrollViewlines.count > Int(maxHeight / 16)maxHeight.fixedSize(horizontal: false, vertical: true)References: WWDC 2023 — Demystify SwiftUI performance,
_FlexFrameLayoutvs_FixedSizeLayout, SE-0373 —letin result buildersReview & Testing Checklist for Human
CI has no macOS build — all Swift changes are unverified by CI. Local Xcode build + manual testing required.
letbindings inside@ViewBuilderbodies compile (SE-0373). All 5 files use this pattern.Notes
lineThreshold: 25is approximate (derived from400pt / codeLineHeight). The actualcodeLineHeightis computed fromNSLayoutManagerfont metrics for DMMono-Regular 13pt.Textview configuration across the if/else branches — if styling changes, both branches must be updated.adaptiveScrollFramemodifier andToolCallChip.swiftwere already fixed in Fix LazyVStack cell hangs: definite scroll heights + cached coloredOutput #24019 — no changes needed.Link to Devin session: https://app.devin.ai/sessions/38404b0c14494b9b97274a5fb6110b25
Requested by: @ashleeradka