perf(chat): add MessageCellHeightCache to eliminate recursive sizeThatFits on completed cells (LUM-720)#23612
Closed
ashleeradka wants to merge 2 commits into
Closed
perf(chat): add MessageCellHeightCache to eliminate recursive sizeThatFits on completed cells (LUM-720)#23612ashleeradka wants to merge 2 commits into
ashleeradka wants to merge 2 commits into
Conversation
…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.
ashleeradka
added a commit
that referenced
this pull request
Apr 4, 2026
…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.
![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: 673898684d
ℹ️ 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".
| && message.toolCalls.allSatisfy { $0.isComplete } | ||
| && !isLatestAssistant | ||
| && highlightedMessageId != message.id | ||
| let cachedHeight = isCellHeightCacheable ? scrollState.cellHeightCache[message.id] : nil |
There was a problem hiding this comment.
Using message.id as the sole cache key means a stale height is reused whenever a message remains cacheable but its intrinsic height changes. That happens in normal flows like window width changes (the bubble width is derived from containerWidth) and on-demand rehydration (ChatViewModel.handleMessageContentResponse updates text/tool content without making the message streaming/pending), so this fixed .frame(height:) can leave rows with incorrect height after those updates. Because the geometry callback now measures the framed size, it cannot self-correct once a stale cached value is applied.
Useful? React with 👍 / 👎.
![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.
…resize Two bugs flagged by Codex (P1) and Devin (red): 1. onGeometryChange was placed AFTER .frame(height: cachedHeight), meaning it always measured the constrained height, not the natural content height. Once a stale value was cached it could never self-correct. Fix: move onGeometryChange before .frame(height:) so SwiftUI measures natural height first, then pins the frame. Any layout input change (showTimestamp toggle, dismissedDocumentSurfaceIds) will write the correct height on the next pass. 2. handleContainerWidthChanged() never cleared cellHeightCache. After a window resize, text reflows to different line counts but all cached heights remain stale. The self-reinforcing loop in (1) meant cells were permanently stuck. Fix: call cellHeightCache.removeAll() on width change so cells re-measure.
ashleeradka
added a commit
that referenced
this pull request
Apr 4, 2026
…ide LazyVStack cells (#23611) * perf(chat): replace maxHeight with definite height on ScrollViews inside 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. * fix: address Codex review — restore content-adaptive height for short 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. * fix: revert maxHeight to height for small ScrollViews — cell height cache 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. * fix: add lineCount > 500 guard to ToolConfirmationBubble.codePreviewBlock 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. * fix: use VCodeView.countLines in ToolConfirmationBubble for consistency Replace .components(separatedBy: .newlines).count (allocates intermediate array) with VCodeView.countLines(in:) (UTF-8 byte scan, no allocation). Aligns with ToolCallChip and ToolCallProgressBar. * refactor: extract adaptiveScrollFrame modifier for LazyVStack height 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.
Comment on lines
+248
to
+254
| .onGeometryChange(for: CGFloat.self) { proxy in | ||
| proxy.size.height | ||
| } action: { height in | ||
| guard isCellHeightCacheable, height > 0 else { return } | ||
| scrollState.cellHeightCache[message.id] = height | ||
| } | ||
| .frame(height: cachedHeight) |
Contributor
There was a problem hiding this comment.
🟡 Stale cached height persists after cell content changes because @ObservationIgnored cache write cannot trigger re-render
When a completed cell's height-affecting content changes (e.g., showTimestamp toggled at MessageCellView.swift:147, or a document surface dismissed), the onGeometryChange action correctly updates scrollState.cellHeightCache[message.id] with the new natural height, but because cellHeightCache is @ObservationIgnored (MessageListScrollState.swift:255), the write does not trigger a SwiftUI re-render. The cell remains rendered at the stale cached height from .frame(height: cachedHeight) until some unrelated state change triggers a body re-evaluation.
The comment on lines 241-247 claims the cache is "self-correcting" and "the pinned frame updates on the following render" — but nothing schedules that "following render." In a static (non-streaming) conversation, a user toggling a timestamp on an older message sees the TimestampDivider content overflow or the cell spacing become incorrect, and this persists until the user scrolls or another event triggers a re-render.
Trigger scenario
- User has a static conversation (not streaming)
- User taps an older message to toggle
showTimestamp state.showTimestampchanges → body evaluatesisCellHeightCacheableis true,cachedHeightreads the stale (pre-timestamp) value.frame(height: cachedHeight)constrains the cell to the wrong heightonGeometryChangefires with the correct (post-timestamp) height, updates the cache- But since
cellHeightCacheis@ObservationIgnored, no re-render is scheduled - Cell stays at wrong height until an unrelated event triggers a re-render
Prompt for agents
The cell height cache in MessageListContentView writes updated heights to an @ObservationIgnored dictionary (scrollState.cellHeightCache) from the onGeometryChange action, but this write cannot trigger a SwiftUI re-render. When a cell's content changes height (e.g., showTimestamp toggled, document surface dismissed), the stale cached height persists on screen until an unrelated re-render occurs.
The fix should ensure that when onGeometryChange detects a height change for a cell that already has a cache entry, the view re-renders to apply the new height. Several approaches:
1. Per-cell cache invalidation: When computing isCellHeightCacheable, also remove the cache entry if a height-affecting input changed since the cache was populated. This could be done by storing a lightweight key (e.g., hash of showTimestamp + dismissedDocumentSurfaceIds intersection) alongside the cached height, and invalidating when the key mismatches.
2. In the onGeometryChange action, when the newly measured height differs from the existing cached value, touch a tracked @Observable property (e.g., increment a version counter) that forces the affected cell's frame to be recomputed on the next render.
3. Invalidate the specific cache entry (removeValue(forKey: message.id)) at the top of the ForEach body when a height-affecting property has changed. For instance, maintain a per-cell fingerprint of (showTimestamp, relevantDismissedSurfaceIds) alongside the cached height, and evict on mismatch.
Approach 1 or 3 are cleanest since they avoid observation-triggered re-render loops. The relevant files are MessageListContentView.swift (ForEach body around line 193-254) and MessageListScrollState.swift (cellHeightCache declaration at line 255).
Was this helpful? React with 👍 or 👎 to provide feedback.
Closed
6 tasks
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a cell height cache to
MessageListScrollStatethat eliminates recursivesizeThatFitstraversal for completed message cells duringLazyVStackmeasurement — the root cause of multi-second hangs during streaming and tool execution.Root Cause
SwiftUI's
LazyVStackcallssizeThatFitson every visible cell on every transaction flush, regardless of whether the cell's body was re-evaluated. With ~7+ layout nodes per cell, this costs O(visible_cells × nesting_depth) per flush. During streaming (every ~16ms), a conversation with 10 completed cells means 10 full subtree measurements per flush.The
Equatable + .equatable()chain we built prevents body re-evaluation — that work is correct. But layout measurement is a separate pass thatLazyVStackalways runs.Fix
Cache the rendered height of each completed cell in
MessageListScrollState.cellHeightCache. Apply cached heights as.frame(height:)sosizeThatFitsreturns immediately without recursing into the cell subtree.A cell is height-cacheable when its rendered size is stable:
All other cells remain uncached and measure normally.
Heights are captured via
.onGeometryChange(for: CGFloat.self)after first render, so cells always display correctly before being cached. Cache is cleared on conversation switch viaMessageListScrollState.reset().Before / After
Files Changed
MessageListScrollState.swift— addscellHeightCache: [UUID: CGFloat], clears on resetMessageListContentView.swift— computesisCellHeightCacheable, applies.frame(height: cachedHeight), captures via.onGeometryChangeWhy This Is Safe
@ObservationIgnoredfield — no SwiftUI observable writes on cache updatereset()callMessageCellView,ChatBubble, or any leaf viewsTesting
Build and send a message that triggers tool calls. After the tool completes and the assistant finishes responding, the conversation should remain fully responsive — no freeze when the next message is sent.