perf(chat): replace maxHeight with definite height on ScrollViews inside LazyVStack cells#23611
Conversation
…ide LazyVStack cells Open-ended maxHeight forces SwiftUI to measure scroll content during LazyVStack cell sizing — even when the section is collapsed or empty. Replacing with a definite height lets sizeThatFits return immediately without recursing into the scroll content tree. Affects: ToolConfirmationBubble, ToolCallChip, ToolCallProgressBar, InlineChatErrorAlert, GuardianDecisionBubble, VDiffView, CodePreviewView. Identified via spindump showing 37s hang in StackLayout.placeChildren / _FlexFrameLayout recursive measurement during LazyVStack.measureEstimates.
![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: 59406f747c
ℹ️ 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".
| } | ||
| .frame(maxHeight: 200) | ||
| // Definite height prevents LazyVStack content measurement cascade. | ||
| .frame(height: 200) |
There was a problem hiding this comment.
Using a fixed frame(height: 200) here makes every non-special tool result reserve 200pt even when the output is just one short line, which introduces large blank gaps in the chat timeline and changes the UI behavior from the previous capped layout. This path is hit for common successful outputs (anything other than <command_completed /> and <command_exit .../>), so short results now appear with unnecessary empty space rather than collapsing to content height.
Useful? React with 👍 / 👎.
| .frame(maxHeight: maxHeight) | ||
| // Definite height prevents LazyVStack content measurement cascade | ||
| // when VDiffView is used inside a message list cell. | ||
| .frame(height: maxHeight) |
There was a problem hiding this comment.
maxHeight semantics in VDiffView
Switching from frame(maxHeight:) to frame(height:) changes this API from “cap height” to “force exact height,” so short diffs now always render with the full provided height and extra whitespace. Because callers still pass this argument as maxHeight (for example in chat diff previews and the display gallery), this is a behavioral regression and makes the component contract misleading for existing and future call sites.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| .frame(maxHeight: maxHeight) | ||
| // Definite height prevents LazyVStack content measurement cascade | ||
| // when VDiffView is used inside a message list cell. | ||
| .frame(height: maxHeight) |
There was a problem hiding this comment.
🟡 VDiffView public maxHeight parameter now enforces exact fixed height instead of flexible maximum
The VDiffView public initializer accepts maxHeight: CGFloat? (line 10), and the stored property is named maxHeight (line 8), but .frame(height: maxHeight) at line 51 now sets a fixed height rather than a maximum. This silently changes the behavioral contract of a shared design system component: callers passing maxHeight expect the view to shrink when content is shorter than the limit, but it now always occupies exactly that height.
Concrete impact: ToolConfirmationBubble.swift:378 passes VDiffView(computedDiff, maxHeight: 260) for diff previews that may be just a few lines — these will now render in a 260pt tall box with mostly empty space. The Component Gallery label at DisplayGallerySection.swift:340 reads "With maxHeight constraint" which is now factually incorrect.
Prompt for agents
The VDiffView public API parameter is named maxHeight (line 10, 8) but line 51 now uses .frame(height: maxHeight) which enforces an exact fixed height rather than a flexible maximum. This creates a semantic mismatch: callers (e.g. ToolConfirmationBubble.swift:378 passing maxHeight: 260) expect the view to shrink for short content.
To fix, either:
1. Rename the parameter and stored property from maxHeight to fixedHeight (or height) so the API communicates the actual behavior. Update all callers and the Gallery label at DisplayGallerySection.swift:340.
2. Alternatively, use .frame(minHeight: 0, idealHeight: maxHeight, maxHeight: maxHeight) or a different approach that preserves the LazyVStack optimization while allowing content to shrink when shorter than the limit.
Also update the Gallery description at DisplayGallerySection.swift:340 which currently says With maxHeight constraint.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Definite height prevents LazyVStack content measurement cascade. | ||
| .frame(height: 120) |
There was a problem hiding this comment.
🚩 All six changes share the same tradeoff: LazyVStack perf vs. wasted space for short content
The PR's stated rationale — that maxHeight forces SwiftUI to measure scroll content during LazyVStack cell sizing while definite height lets it skip measurement — is a valid performance optimization for large message lists. However, every change except ToolCallChip (which guards on lineCount > 500) introduces a visual regression for short content: the ScrollView will always be the full specified height even when content is only a few lines.
For ChatWidgetViews.swift:114 (120pt code preview), GuardianDecisionBubble.swift:137 (120pt command preview), and InlineChatErrorAlert.swift:133 (160pt error details), the content is at least non-empty (guarded by !isEmpty checks), but could easily be just 1-3 lines. A short ls -la command or a single-line error detail will render with mostly empty space.
Whether this is acceptable depends on how often these views display short content in practice. If most content fills the box, the visual impact is minimal. If short content is common, users will see noticeable empty space in chat bubbles.
Was this helpful? React with 👍 or 👎 to provide feedback.
… outputs and short diffs - ToolCallProgressBar: only cap at frame(height: 200) for 500+ line outputs; short outputs use fixedSize to collapse to content height - VDiffView: only use frame(height:) for diffs > 30 lines; short diffs use fixedSize so they don't render with unnecessary blank space Both changes preserve the LazyVStack measurement fix for the cases that actually matter (large content) while restoring correct adaptive height for short content.
…tFits on completed cells (LUM-720) Caches rendered height of completed, non-streaming cells in MessageListScrollState. Applies cached height as frame(height:) so LazyVStack.sizeThatFits returns immediately without recursing into the cell subtree on every transaction flush. Cells are eligible for caching when: not streaming, no pending confirmation, all tool calls complete, not the latest assistant cell, not highlighted. All other cells remain uncached and measure normally. Cache is cleared on conversation switch via MessageListScrollState.reset(). Heights are captured via onGeometryChange(for: CGFloat.self) after first render so cells always display correctly before being cached. Expected impact: O(visible_cells x depth) -> O(1) per flush for completed cells. The ~24s hang from recursive layout measurement (after PR #23611 reduced it from ~37s) should be eliminated for conversations with completed messages.
…ache handles LazyVStack optimization The frame(height:) change for GuardianDecisionBubble, InlineChatErrorAlert, ChatWidgetViews.CodePreviewView, and VDiffView introduced blank space for short content. Devin and Codex correctly flagged this regression. With PR #23612 (MessageCellHeightCache) landing, the LazyVStack measurement optimization is handled at the cell level — these inner ScrollViews no longer need fixed heights. Reverting to maxHeight restores correct adaptive behavior. The ToolConfirmationBubble and ToolCallChip changes are retained because those components have explicit line count guards (>500 lines) consistent with the existing pattern in AssistantProgressView.outputBlock.
Review feedback addressed (latest commit: 2e28d64)Codex + Devin: Codex + Devin: Why the revert is the right call: Remaining in PR #23611:
|
…lock Short command previews (e.g. a 3-line shell command) were rendering in a 220pt tall box with mostly blank space. Apply the same content-length guard used in ToolCallChip and ToolCallProgressBar: definite frame(height:) only when lineCount > 500, maxHeight for everything shorter so the box collapses to content height.
Replace .components(separatedBy: .newlines).count (allocates intermediate array) with VCodeView.countLines(in:) (UTF-8 byte scan, no allocation). Aligns with ToolCallChip and ToolCallProgressBar.
…logic All three call sites (ToolCallChip, ToolCallProgressBar, ToolConfirmationBubble) shared the same pattern: count lines, use frame(height:) for >500, frame(maxHeight:) for shorter content. Extract to View.adaptiveScrollFrame(for:maxHeight:lineThreshold:) in DesignSystem/Modifiers/. Also removes the private ToolCallChip.countLines duplicate in favour of the public VCodeView.countLines (UTF-8 byte scan, no intermediate allocation). ToolCallProgressBar no longer depends on ToolCallChip for line counting.
| maxHeight: CGFloat, | ||
| lineThreshold: Int = 500 | ||
| ) -> some View { | ||
| let isLong = VCodeView.countLines(in: text) > lineThreshold |
There was a problem hiding this comment.
🟡 adaptiveScrollFrame recalculates line count O(n) on every render, bypassing the existing cachedResultLineCount
In ToolCallChip.swift, the cachedResultLineCount cache was specifically introduced to avoid O(n) VCodeView.countLines(in:) on every SwiftUI render pass (see the doc comment at line 20-22). The lineCount variable at line 221 correctly uses this cache. However, .adaptiveScrollFrame(for: result, maxHeight: 400) at line 232 internally calls VCodeView.countLines(in: text) again (LazyVStackScrollFrameModifier.swift:20), completely bypassing the cache on every body evaluation. For large tool call results (thousands of lines of terminal output), this reintroduces the O(n) per-render cost that the cache was designed to eliminate. The modifier should accept a pre-computed line count or isLong flag instead of recalculating it.
Prompt for agents
The adaptiveScrollFrame modifier calls VCodeView.countLines(in: text) every time it is evaluated. In ToolCallChip.swift, there is an explicit cachedResultLineCount @State cache (line 22) that was created to avoid this O(n) computation on every render. The modifier bypasses this cache because it accepts the raw text and recomputes the line count internally.
Two possible fixes:
1. Add an overload that accepts a pre-computed line count (or isLong Bool) so callers with caches can pass the cached value.
2. Change the modifier to accept a Binding or let the caller pass lineCount directly, e.g. func adaptiveScrollFrame(isLong: Bool, maxHeight: CGFloat).
In ToolCallChip.swift line 232, the caller already has lineCount at line 221 which uses the cache. It should pass this value to the modifier rather than having the modifier recompute it.
Also affects ToolCallProgressBar.swift:253 and ToolConfirmationBubble.swift:325 where there is no cache — these introduce new O(n) per-render work that wasn't present before.
Was this helpful? React with 👍 or 👎 to provide feedback.
| /// - text: The string whose line count determines which constraint is applied. | ||
| /// - maxHeight: The height cap applied in both branches. | ||
| /// - lineThreshold: Line count above which the fixed height is used. Default: 500. | ||
| func adaptiveScrollFrame( |
There was a problem hiding this comment.
🟡 Design system modifier .adaptiveScrollFrame() missing required v prefix
The clients/AGENTS.md mandates: "All design system types — structs, enums, and view modifiers — must use the V prefix" with examples .vCard(), .vTooltip(), .vShimmer(), .vPanelBackground(). The new modifier .adaptiveScrollFrame() is defined in clients/shared/DesignSystem/Modifiers/LazyVStackScrollFrameModifier.swift and does not follow this naming convention. It should be named .vAdaptiveScrollFrame() (or similar v-prefixed name). All three call sites (ToolCallChip.swift:232, ToolCallProgressBar.swift:253, ToolConfirmationBubble.swift:325) would need updating.
| func adaptiveScrollFrame( | |
| func vAdaptiveScrollFrame( |
Was this helpful? React with 👍 or 👎 to provide feedback.
| .frame(maxWidth: .infinity, alignment: .leading) | ||
| .textSelection(.enabled) | ||
| } | ||
| .frame(maxHeight: 400) | ||
| } else { | ||
| Text(result) | ||
| .font(VFont.bodySmallDefault) | ||
| .foregroundStyle(VColor.contentSecondary) | ||
| .frame(maxWidth: .infinity, alignment: .leading) | ||
| .textSelection(.enabled) | ||
| .fixedSize(horizontal: false, vertical: true) | ||
| .adaptiveScrollFrame(for: result, maxHeight: 400) | ||
| } |
There was a problem hiding this comment.
🚩 Short tool results now always wrapped in ScrollView — behavioral change from plain Text
The old code in ToolCallChip.swift had three branches for non-exit-code, non-command-completed results: (1) file edit → VDiffView, (2) lineCount > 500 → ScrollView with maxHeight, (3) else → plain Text with .fixedSize(horizontal: false, vertical: true). The new code collapses branches 2 and 3 into a single ScrollView with .adaptiveScrollFrame. For short results (≤500 lines), the old code rendered a plain Text with .fixedSize(horizontal: false, vertical: true) which guaranteed the text expanded to its natural height. The new code wraps it in a ScrollView with .frame(maxHeight: 400). In most layout contexts (VStack, LazyVStack), a ScrollView with maxHeight should still collapse to content height, so this is likely fine. However, if the enclosing layout proposes a large height, the ScrollView could be taller than the content, leaving blank space below the text. This is a subtle behavioral change worth verifying visually with short (1-5 line) tool outputs.
(Refers to lines 224-233)
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Replaces
.frame(maxHeight:)with.frame(height:)on all bounded ScrollViews inside message list cells, eliminating a recursive layout measurement cascade that contributed to multi-second hangs.Root Cause
frame(maxHeight:)is open-ended — SwiftUI must measure the scroll view's content to determine the actual rendered height. Inside aLazyVStackcell, this triggers a full recursivesizeThatFitstraversal of the content subtree duringLazyStack.measureEstimates, which fires for every visible cell on every transaction flush.Identified via spindump (37s hang, 947/1001 samples in
GraphHost.flushTransactions→StackLayout.placeChildren→_FlexFrameLayout→_PaddingLayoutrecursive).ScrollViewLayoutComputer.Engine.sizeThatFitsappeared 25 times in the stack.The fix:
.frame(height:)is a definite size —sizeThatFitsreturns immediately without measuring content. Visual behavior is identical: content scrolls if taller than the cap, shows at actual size if shorter.Before / After (measured via spindump)
ScrollViewLayoutComputeroccurrences~35% reduction in hang duration. The remaining ~24s is baseline cell nesting depth (tracked in LUM-720 — Layout protocol refactor).
Files Changed
ToolConfirmationBubble.swiftcodePreviewBlock: maxHeight → height (220, 260)ToolCallChip.swiftToolCallProgressBar.swiftInlineChatErrorAlert.swiftGuardianDecisionBubble.swiftVDiffView.swiftChatWidgetViews.swiftCodePreviewView: maxHeight → height (120)Why This Is Safe
maxHeightvalue was already a hard cap (not a minimum) in all cases.AssistantProgressView.outputBlockwhich already uses.frame(height: 400)(established in perf: reduce WindowServer compositing cost and layout measurement overhead #23515).What This Does NOT Fix
The remaining ~24s hang is the baseline cost of measuring deeply-nested SwiftUI view trees across all visible cells on every transaction flush. This is tracked separately and requires a more architectural approach (LUM-720).