Skip to content

perf(chat): add MessageCellHeightCache to eliminate recursive sizeThatFits on completed cells (LUM-720) #23612

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ashleeradka wants to merge 2 commits into from
Closed

perf(chat): add MessageCellHeightCache to eliminate recursive sizeThatFits on completed cells (LUM-720) #23612

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6738986
perf(chat): add MessageCellHeightCache to eliminate recursive sizeTha…
ashleeradka Apr 4, 2026
151ea5e
fix: make cell height cache self-correcting and invalidate on window …
ashleeradka Apr 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions clients/macos/vellum-assistant/Features/Chat/MessageListContentView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,18 @@ struct MessageListContentView: View, Equatable {
let cellActivePendingRequestId: String? =
(message.confirmation != nil || !message.toolCalls.isEmpty)
? state.activePendingRequestId : nil
// A cell is height-cacheable when its rendered size is stable:
// not streaming, no pending confirmation, all tool calls done,
// not the latest assistant cell (may still grow), not highlighted.
// Exempt cells always re-measure; cached cells get a definite
// frame(height:) so LazyVStack.sizeThatFits returns immediately
// without recursing into the cell subtree.
let isCellHeightCacheable = !message.isStreaming
&& message.confirmation?.state != .pending
&& message.toolCalls.allSatisfy { $0.isComplete }
&& !isLatestAssistant
&& highlightedMessageId != message.id
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
let cachedHeight = isCellHeightCacheable ? scrollState.cellHeightCache[message.id] : nil
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Invalidate cached heights when cell layout inputs change

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 👍 / 👎.

MessageCellView(
message: message,
showTimestamp: state.showTimestamp.contains(message.id),
Expand Down Expand Up @@ -226,6 +238,20 @@ struct MessageListContentView: View, Equatable {
providerCatalogHash: providerCatalogHash
)
.equatable()
// Measure natural content height BEFORE applying the cached frame
// so the cache is always updated from unconstrained layout, not
// from the pinned frame. This makes the cache self-correcting:
// if anything changes cell height (window resize, showTimestamp
// toggle, dismissedDocumentSurfaceIds) the next layout pass
// writes the correct height and the pinned frame updates on the
// following render.
.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)
Comment on lines +248 to +254
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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
  1. User has a static conversation (not streaming)
  2. User taps an older message to toggle showTimestamp
  3. state.showTimestamp changes → body evaluates
  4. isCellHeightCacheable is true, cachedHeight reads the stale (pre-timestamp) value
  5. .frame(height: cachedHeight) constrains the cell to the wrong height
  6. onGeometryChange fires with the correct (post-timestamp) height, updates the cache
  7. But since cellHeightCache is @ObservationIgnored, no re-render is scheduled
  8. 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).
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

}

ForEach(state.orphanSubagents) { subagent in
Expand Down
8 changes: 8 additions & 0 deletions clients/macos/vellum-assistant/Features/Chat/MessageListScrollState.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,13 @@ final class MessageListScrollState {

// MARK: - Layout Cache Fields

/// Cached rendered heights for completed, non-streaming message cells.
/// Applied as `.frame(height:)` so `LazyVStack.sizeThatFits` returns
/// immediately without recursing into the cell subtree on every flush.
/// Keyed by message ID; invalidated on conversation switch and when a
/// message transitions back to active (streaming, pending confirmation).
@ObservationIgnored var cellHeightCache: [UUID: CGFloat] = [:]

@ObservationIgnored var cachedLayoutKey: PrecomputedCacheKey?
@ObservationIgnored var cachedLayoutMetadata: CachedMessageLayoutMetadata?
@ObservationIgnored var messageListVersion: Int = 0
Expand Down Expand Up @@ -593,6 +600,7 @@ final class MessageListScrollState {
if _isPaginationInFlight { _isPaginationInFlight = false }
wasPaginationTriggerInRange = false
lastPaginationCompletedAt = .distantPast
cellHeightCache.removeAll()
cachedLayoutKey = nil
cachedLayoutMetadata = nil
cachedDerivedStateBox = nil
Expand Down
3 changes: 3 additions & 0 deletions clients/macos/vellum-assistant/Features/Chat/MessageListView+Lifecycle.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@ extension MessageListView {
func handleContainerWidthChanged() {
guard containerWidth > 0, abs(containerWidth - scrollState.lastHandledContainerWidth) > 2 else { return }
scrollState.lastHandledContainerWidth = containerWidth
// Cell heights are width-dependent (text reflows). Invalidate so cells
// re-measure at their natural height on the next layout pass.
scrollState.cellHeightCache.removeAll()
resizeScrollTask?.cancel()
resizeScrollTask = Task { @MainActor [scrollState] in
scrollState.beginStabilization(.resize)
Expand Down