Fix LazyVStack cell hangs: definite scroll heights + cached coloredOutput#24019
Conversation
…an line counting, @State caching - Add charThreshold parameter (default 50,000) to adaptiveScrollFrame modifier to catch single-line mega-strings (base64 data, minified JSON) that bypass the line-count check but still trigger expensive Core Text width measurement. - Replace components(separatedBy:) with O(1)-memory byte scan in AssistantProgressView.outputBlock, eliminating N String allocations per render. Also add the character-count safety net here. - Add @State line-count caching to ToolCallProgressBar and ToolConfirmationBubble, matching the existing pattern in ToolCallChip. Caches are populated on expand and invalidated on collapse, so the O(n) scan runs once per content change instead of on every SwiftUI render pass. 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:
|
Add optional lineCount parameter to adaptiveScrollFrame so callers with @State-cached values bypass the internal countLines scan. Previously the cached values were written but never read — adaptiveScrollFrame always recomputed internally, making the caching pure overhead. Now ToolCallProgressBar, ToolConfirmationBubble, and ToolCallChip all pass their cached line count through, achieving the intended O(1) re-render optimization. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
Swift's @ViewBuilder result builder does not allow control flow statements like 'for' loops. Replace the byte-scan for loop with an equivalent reduce expression that produces the same line count in a single let binding. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
codePreviewBlock is called from two sites with different content strings (inlinePreview and diffDisclosure), but they shared a single @State cachedPreviewLineCount. Whichever appeared first would cache its line count, and the other would reuse the wrong value. Fix: remove the caching entirely. The inline countLines inside adaptiveScrollFrame is O(n) with O(1) memory and preview/diff content is typically short — caching saves negligible time and isn't worth the shared-state bug risk. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
ashleeradka
left a comment
There was a problem hiding this comment.
Code Review — Vex ✦
1. text.count runs O(n) on every render inside the modifier
// LazyVStackScrollFrameModifier.swift
let isLong = lines > lineThreshold || text.count > charThresholdtext.count iterates grapheme clusters — on a 100KB tool output this runs every body evaluation. The line count can be cached via the new lineCount: pass-through, but the char count cannot. For consistency, consider either:
- Adding a
charCount: Int?pass-through parameter (mirrors lineCount pattern) - Or at minimum, short-circuit: check
lines > lineThresholdfirst and skip the char check when it's already true
Since most large outputs will exceed the line threshold, the char check only runs on pathological single-line content — so this is likely fine in practice. But it's worth a conscious decision.
2. Stale cache risk in ToolCallProgressBar
.onChange(of: expandedStepId) { ... }There's no .onChange(of: toolCalls) handler. If a tool call's result updates while the step is already expanded (e.g. a long-running tool streaming partial results), cachedResultLineCount will be stale. ToolCallChip handles this with .onChange(of: toolCall.result) — should this view do the same?
If result-update-while-expanded isn't realistic for the progress bar, a comment explaining why would be helpful.
3. ToolCallChip.countLines used as a utility across views
ToolCallProgressBar and ToolConfirmationBubble both call ToolCallChip.countLines(in:). This works because they're in the same module, but countLines is a string utility, not a ToolCallChip concern. If this pattern spreads further it should move to a shared String extension or utility. Not blocking, but noting for future cleanup.
4. outputBlock in AssistantProgressView duplicates the adaptive logic
// AssistantProgressView.swift — outputBlock
let isLong = lineCount > 500 || copyText.count > 50_000
if isLong {
ScrollView { ... }.frame(height: 400)
}This is an inline reimplementation of adaptiveScrollFrame with hardcoded thresholds rather than using the shared modifier. The charThreshold logic is now in two places. Not a regression (pre-existing), but worth consolidating in a follow-up so threshold changes only need to happen in one place.
…tep switch Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
|
Addressing each point: 1.
|
…Block Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
QA Result — Vex ✦Test: expand tool result with 2000 lines (
Root cause found: // AssistantProgressView.swift:1090
private func coloredOutput(_ result: String, isError: Bool) -> AttributedString {
let lines = result.components(separatedBy: "\n") // ← N String allocations
// ... then appends 2000+ AttributedString fragments in a loop
}This is called at line 991 BEFORE the result reaches outputBlock(
text: nil,
attributedText: coloredOutput(result, isError: toolCall.isError), // ← expensive, every body eval
copyText: result,
copyLabel: "Copy output"
)So even though The fix in this PR (byte-scan line counting + charThreshold) is correct but insufficient — it optimizes the line count check but doesn't address the dominant cost, which is the AttributedString construction happening unconditionally on every body evaluation. Suggested fix direction:
|
…very render Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
|
Great QA find — What changed:
For a 2000-line output, this eliminates:
Short content (< 500 lines) still gets diff coloring via |
Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
…State Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
|
Implemented the simplified direction in
All outputs now get both max-height capping AND diff coloring. No line-count gating for either decision. Changes to ToolCallChip/ToolCallProgressBar/ToolConfirmationBubble/LazyVStackScrollFrameModifier kept as independently valuable. |
Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
|
@devin-ai-integration The previous direction (always Root Cause
The original code on main was architecturally correct:
Your change replaced both paths with New Direction1. Revert
|
…Full, fix adaptiveScrollFrame - outputBlock: restore two-path branching (long >30 lines -> ScrollView.frame(height:400), short -> outputTextView directly, no ScrollView). Threshold lowered from 500 to 30 lines. - resolvedInputFull: add bounded display for Technical Details section (ScrollView.frame(height:300) for long inputs, plain Text for short). - adaptiveScrollFrame: remove .frame(maxHeight:) from short path — it creates _FlexFrameLayout that recursively measures children inside LazyVStack cells. Short content gets no height constraint. Default lineThreshold lowered from 500 to 30. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
|
Implemented all 4 changes in
Remaining CI passed (2 pass, 5 skipped). Needs local Xcode build + the 6-point testing checklist. |
Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
…ht/lineHeight ToolCallProgressBar: lineThreshold 200 → 12 (200pt / 16pt per line) ToolConfirmationBubble: lineThreshold derived from maxHeight / 16 Content that naturally fits within maxHeight renders at natural size. Content that would exceed maxHeight gets the fixed-height ScrollView path. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
…tput 1. Replace text.count (O(n) grapheme clusters) with text.utf8.count (O(1)) in LazyVStackScrollFrameModifier, outputBlock, and resolvedInputFull. 2. Cache resolvedInputFull isLong flag via @State — avoids O(n) byte scan in the view body on every render. 3. Eagerly compute cachedColoredResult in .onChange(of: isDetailExpanded) so the first render of expanded content has colored output — eliminates the plain-to-colored flash on first expand. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
| .onChange(of: toolCall.result) { _, newResult in | ||
| if let result = newResult, !result.isEmpty { | ||
| cachedColoredResult = coloredOutput(result, isError: toolCall.isError) | ||
| } else { | ||
| cachedColoredResult = nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 cachedInputIsLong is never invalidated when toolCall.inputFull changes after rehydration
cachedInputIsLong is populated in .onChange(of: isDetailExpanded) (line 933) and .onAppear (line 1039), but unlike cachedColoredResult which has .onChange(of: toolCall.result) (line 1044) to handle data changes, there is no corresponding .onChange(of: toolCall.inputFullLength) to invalidate cachedInputIsLong. When a step is expanded before rehydration completes, the cache is set based on the truncated/empty input. After onRehydrate delivers the full (potentially very long) input, cachedInputIsLong retains its stale value (nil or false). The view body then evaluates let inputIsLong = cachedInputIsLong ?? false → false at line 968, causing long rehydrated input to render as unconstrained Text instead of the intended 300pt ScrollView. Compare with ToolCallChip which correctly invalidates cachedInputFull via .onChange(of: toolCall.inputFull) at clients/shared/Features/Chat/ToolCallChip.swift:298.
| .onChange(of: toolCall.result) { _, newResult in | |
| if let result = newResult, !result.isEmpty { | |
| cachedColoredResult = coloredOutput(result, isError: toolCall.isError) | |
| } else { | |
| cachedColoredResult = nil | |
| } | |
| } | |
| .onChange(of: toolCall.result) { _, newResult in | |
| if let result = newResult, !result.isEmpty { | |
| cachedColoredResult = coloredOutput(result, isError: toolCall.isError) | |
| } else { | |
| cachedColoredResult = nil | |
| } | |
| } | |
| .onChange(of: toolCall.inputFullLength) { | |
| // Invalidate so the next render recalculates for rehydrated input. | |
| cachedInputIsLong = nil | |
| } |
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. |
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>
…les (#24091) * fix: replace ScrollView.frame(maxHeight:) with definite heights in 4 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> * fix: apply two-path pattern to VDiffView for short diffs 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> * fix: add single-line charThreshold guard to all three two-path views 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> * fix: apply two-path pattern to CodeBlockView for capped height in LazyVStack 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> * fix: use conservative divisor for VDiffView threshold, compute CodeBlockView 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> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: ashlee@vellum.ai <ashlee@vellum.ai>
…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
Fixes main-thread hangs caused by
_FlexFrameLayoutrecursive measurement insideLazyVStackcells. The root cause:.frame(maxHeight:)compiles to_FlexFrameLayout, which must measure all children before clamping — O(n × depth) work per cell..frame(height:)compiles to_FixedSizeLayout, which returns immediately — O(1).The rule: inside a
LazyVStackcell, aScrollViewmust have a definiteframe(height:)or noScrollViewat all. Never.frame(maxHeight:).Why this is needed
A spindump captured a 50-second hang with the main thread stuck in SwiftUI's recursive
sizeThatFitschain —LazyVStackmeasuring cells that use.frame(maxHeight:). Each cell triggers_FlexFrameLayout.sizeThatFits→ recursive child measurement through nested VStack/HStack hierarchies, 5–6 levels deep. Additionally,coloredOutput(which builds anAttributedStringwith N substring allocations) was recomputed inline on every SwiftUI render pass, andoutputBlockused.components(separatedBy: "\n").countwhich allocates NStringobjects per call.What changed
adaptiveScrollFramemodifier — Removed.frame(maxHeight:)from the short-content path. Short content now gets no height constraint (ScrollView collapses naturally). Lowered defaultlineThresholdfrom 500 → 30 (~480pt at 16pt line height). AddedcharThreshold(50,000) for single-line mega-strings usingutf8.count(O(1) for native Swift strings). AddedlineCount:pass-through so callers with cached values skip the internal scan.outputBlock(AssistantProgressView) — Two-path branching: long content (>30 lines or >50k single-line UTF-8 bytes) →ScrollView.frame(height: 400); short →outputTextViewdirectly, no ScrollView. Replaced.components(separatedBy:).countwithutf8.reducebyte-scan (zero allocations). Removed.fixedSize(horizontal: false, vertical: true)from the short path.cachedColoredResult(AssistantProgressView) —coloredOutputis now cached via@State, computed eagerly in.onChange(of: isDetailExpanded)before the expanded body first evaluates, and also in.onAppear/.onChange(of: toolCall.result). All outputs get diff coloring regardless of length.cachedInputIsLong(AssistantProgressView) — TheresolvedInputFullline-count + char-count check is cached via@State, populated eagerly in.onChange(of: isDetailExpanded)and.onAppear. The view body reads only the cached boolean — no O(n) scan on re-render.resolvedInputFull(Technical Details) — Previously used.fixedSize(horizontal: false, vertical: true)with no height cap. Afile_editwith a large parameter could stretch the cell unbounded. Now uses the same two-path pattern: long →ScrollView.frame(height: 300), short → plainText.@Statecaching (shared components) —ToolCallProgressBarcaches line count, passes to modifier, watchesresultLengthfor rehydration.ToolCallChippasses its existing cachedlineCountthrough to the modifier.lineThresholdalignment (ToolCallProgressBar, ToolConfirmationBubble) — Since the short-content path no longer applies.frame(maxHeight:), callers must setlineThresholdso that content exceeding theirmaxHeightenters the fixed-height path.ToolCallProgressBaruseslineThreshold: 12(200pt ÷ 16pt/line).ToolConfirmationBubble.codePreviewBlockderives it dynamically asInt(maxHeight / 16)— formaxHeight: 220→ 13, formaxHeight: 260→ 16. Content under the threshold naturally fits withinmaxHeight, so no cap is needed.Why it's safe
frame(height:)), which_FixedSizeLayoutresolves in O(1) — no child measurement.outputBlock) or no height constraint (inadaptiveScrollFrame) —LazyVStackmeasures only the text, not a scroll container with flexible bounds..frame(maxHeight:)was the only source of_FlexFrameLayoutin these paths.lineThresholdis aligned with itsmaxHeightat ~16pt per line, so content under the threshold naturally fits within the intended height cap. If actual line height differs from 16pt, there may be slight over/under-sizing — verify during testing.coloredOutputcaching means the heavyAttributedStringconstruction runs once, not per-render.References
String.components(separatedBy:)— allocates an array of substrings;utf8.reduceavoids this.@State— SwiftUI-managed storage that persists across re-renders; used here to cachecoloredOutput, line counts, and input sizing flags.String.utf8.count— O(1) on native Swift strings, used instead ofString.count(which is O(n) for extended grapheme cluster counting).Alternatives considered and rejected
.frame(maxHeight:)as universal path_FlexFrameLayout→ recursive measurement → 50-second hang. This was the original bug.coloredOutputfor long contentToolCallDataoutputBlock: 30 lines,ToolCallProgressBar: 12 lines,ToolConfirmationBubble: derived from maxHeight). A single pre-computed boolean can't serve all callers.NSString.enumerateLinesStringper line likecomponents(separatedBy:).utf8.reducebyte-scan is zero-allocation.MessageCellHeightCache(#23612)countLinesintoStringUtilsString.countfor char thresholdutf8.countis O(1) on native Swift strings.Follow-up issues
countLinesinto shared String utilityoutputBlockwithadaptiveScrollFramemodifierScrollView.frame(maxHeight:)instances in LazyVStack cells:CodePreviewView,InlineChatErrorAlert,GuardianDecisionBubble,VDiffViewReview & Testing Checklist for Human
CI has no macOS build — all Swift changes are unverified by CI. Local Xcode build + manual testing required.
letbindings inside@ViewBuildercontexts (let inputIsLong = cachedInputIsLong ?? falsefollowed byif inputIsLonginstepDetailContent;utf8.reduceinoutputBlock). Result builder closures are strict about control flow..onChange(of: isDetailExpanded))file_editinput → Technical Details section should cap at 300pt, scrollablelineThreshold: 12) — results >12 lines should cap at 200pt and scroll. Results ≤12 lines should render at natural height. Verify no excessively tall cellsisErrorpath for both short and long contentNotes
cachedInputIsLongis not invalidated when input changes. IfresolvedInputFullchanges after rehydration (viaonRehydrate), the cached flag will be stale. In practice, rehydration replaces truncated input with the full version — which would only changeisLongfromfalse→trueif the full input crosses the threshold. If this is a concern, add.onChange(of: toolCall.inputFull)to invalidatecachedInputIsLong..onChange(of: isDetailExpanded)handler callscoloredOutputsynchronously on the main thread. For very large outputs (2000+ lines), this may cause a brief hitch on first expand — but it runs once, not per-render.lineThresholdalignment is approximate. Actual line heights depend on font metrics and padding. If lines render taller than 16pt, some content under the threshold may slightly exceedmaxHeightbefore collapsing naturally — this is cosmetic, not a performance issue.coloredOutputatAssistantProgressView.swift:1132still usescomponents(separatedBy: "\n")internally (allocating N substrings). Since it now runs once (cached), this is acceptable perf-wise but is inconsistent with the byte-scan pattern used elsewhere.Link to Devin session: https://app.devin.ai/sessions/ce33d2a1df3a4ab8b3df36dd7f7714a9
Requested by: @ashleeradka