fix(chat): Remove remaining FlexFrame anti-patterns from LazyVStack cell views#25947
Conversation
🤖 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:
|
Replace .frame(maxWidth: .infinity, alignment: .leading) with
HStack { content; Spacer(minLength: 0) } in cell-hierarchy views:
- CodePreviewView (ChatWidgetViews.swift)
- InlineChatErrorAlert (shared)
- GuardianDecisionBubble (shared)
Replace .frame(maxWidth: .infinity) with parent-constrained layout:
- InlineVideoEmbedCard — remove redundant maxWidth (ZStack with
RoundedRectangle shape already fills parent width)
Replace .frame(maxWidth: 360) with .frame(width: 360):
- InlineAudioAttachmentView
- InlineVideoAttachmentView
Remove redundant .frame(maxWidth: .infinity, maxHeight: .infinity):
- InlineVideoEmbedCard fallbackPlaceholder (Color fills ZStack)
- InlineVideoAttachmentView placeholder/loading/failed views
(already inside bounded ZStack with definite dimensions)
These _FlexFrameLayout wrappers trigger alignment-cascade queries
through the entire LazyVStack subtree during sizeThatFits — O(n × depth)
per layout pass, contributing to the 5-second hang on scroll-up.
Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
…f .topTrailing ZStack The parent ZStack used .topTrailing alignment for the save button overlay. Removing .frame(maxWidth/maxHeight: .infinity) from content subviews caused them to shrink to intrinsic size and position at top-right. Fix: Change ZStack to default (.center) alignment and move the save button into .overlay(alignment: .topTrailing). Content subviews now center naturally within the ZStack (RoundedRectangle shape fills the proposed size), and the save button is positioned independently via the overlay modifier — no FlexFrame needed on any child. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
1. Revert frame(width: 360) back to frame(maxWidth: 360) on both InlineVideoAttachmentView and InlineAudioAttachmentView. Bounded maxWidth: 360 is NOT the problematic .infinity FlexFrame pattern, and prevents clipping when the chat pane is narrower than 360pt. 2. Add Color.clear to placeholderView, loadingView, and failedView ZStacks so they expand to fill the parent-proposed size. Without this, VStack-based content would shrink to intrinsic size, reducing the contentShape tap target to just the icon/text cluster instead of the full card area. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
The outer HStack lost its .frame(maxWidth: .infinity) but had no flexible child to expand it. Add Spacer(minLength: 0) after the VStack content so the HStack fills available width, matching the pattern used elsewhere in this PR. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
07c1e4a to
6758e1f
Compare
ashleeradka
left a comment
There was a problem hiding this comment.
Review: PR #25947 — Remove remaining FlexFrame anti-patterns from LazyVStack cell views
Summary
APPROVE — This is a well-executed, thorough cleanup of remaining .frame(maxWidth: .infinity) FlexFrame anti-patterns across 6 LazyVStack cell view files. The PR correctly applies the established HStack(spacing: 0) { content; Spacer(minLength: 0) } pattern from PR #25844 and the Color.clear greedy-fill pattern for maxHeight cases.
Correctness Verification
ChatWidgetViews.swift (CodePreviewView): ✅ Both .frame(maxWidth: .infinity, alignment: .leading) on Text inside ScrollView (line 114) and outside (line 122) correctly replaced with HStack + Spacer. Padding stays on the HStack, preserving insets. The .frame(height: 120) definite height from PR #24091 is preserved.
InlineAudioAttachmentView.swift: ✅ The VStack with .frame(maxWidth: .infinity, alignment: .leading) at line 109 was inside an outer HStack(spacing: VSpacing.sm). Wrapping it in HStack { VStack; Spacer } is correct — the inner HStack is greedy, claims remaining space, VStack left-aligns within it. The bounded .frame(maxWidth: 360) at line 139 is intentionally preserved.
InlineVideoAttachmentView.swift: ✅ Three changes, all correct:
ZStack(alignment: .topTrailing)→ZStack+.overlay(alignment: .topTrailing)for the save button — smart restructuring. The save button was the only child needing top-trailing positioning.placeholderView,loadingView,failedView:.frame(maxWidth: .infinity, maxHeight: .infinity)replaced withZStack { Color.clear; ... }. Color.clear is greedy by default, filling parent-proposed size without FlexFrame measurement.- Bounded
.frame(maxWidth: 360)preserved (not the problematic.infinitypattern).
InlineVideoEmbedCard.swift: ✅ Two removals: outer .frame(maxWidth: .infinity) on the ZStack body, and .frame(maxWidth: .infinity, maxHeight: .infinity) on fallbackPlaceholder. The VColor.auxBlack.opacity(0.8) is a Color (greedy by default), so removing the frame is safe. The .frame(height: cardHeight) stays.
GuardianDecisionBubble.swift: ✅ Three FlexFrames removed — two on Text branches (long/short), one on the outer Group. The outer .frame(maxWidth: .infinity, alignment: .leading) is replaced by HStack+Spacer inside each branch, which are greedy and achieve the same fill behavior.
InlineChatErrorAlert.swift: ✅ Three removals — two on details Text branches (same pattern as above), and the outer .frame(maxWidth: .infinity, alignment: .leading) on the body HStack replaced by adding Spacer(minLength: 0) as a third child. Since this is already an HStack(alignment: .top), adding a Spacer makes it greedy without FlexFrame.
History Check — Not Circular
- PR #24091 (Apr 7) fixed
ScrollView.frame(maxHeight:)in these same files — that fixed the height anti-pattern. This PR fixes the remaining width anti-patterns. Complementary, not circular. - PR #25844 (LUM-835) established the HStack+Spacer replacement pattern. Never reverted.
- PR #25304 (Sidd, Apr 13) fixed InlineChatErrorAlert stretching — different issue (layoutPriority + accent bar Shape), no overlap.
- No reverts found in the history of any of these files for the HStack+Spacer pattern.
Missed Opportunities — Flag for Follow-Up
The broader scan found additional .frame(maxWidth: .infinity) instances inside LazyVStack cells that this PR did NOT address:
- ToolCallChip.swift lines 161, 172, 180 —
.frame(maxWidth: .infinity)on Image views. Used inChatBubble.swift:859→ inside LazyVStack cells. Same anti-pattern family. - InlineAppCreatedCard.swift line 28 —
.frame(maxWidth: .infinity). Used viaInlineSurfaceRouter→ChatBubble.swift:391→ inside cells. - ToolCallProgressBar.swift lines 198, 204 —
.frame(maxWidth: .infinity). Used in tool output rendering inside cells.
These are lower-frequency (tool calls, app cards) but still technically in the LazyVStack cell tree. Worth a follow-up PR.
Minor: Indentation
The failedView in InlineVideoAttachmentView has the if case .port_missing block not re-indented after nesting inside ZStack { Color.clear; VStack { ... } }. The PR description already flags this as a review checklist item. Cosmetic only — not blocking.
Verdict
Approve. All 13 FlexFrame instances are correctly eliminated. The replacement patterns are mechanical, layout-equivalent, and follow the established convention from PR #25844. No circular fixes, no conflicts with recent work. The 3 additional instances in ToolCallChip/ToolCallProgressBar/InlineAppCreatedCard should be filed as a follow-up.
Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
|
Thanks for the thorough review. Indentation fix: Pushed in fd3bfc8 — Follow-up instances acknowledged: Will file a follow-up for the 3 additional
|
| .padding(.trailing, VSpacing.lg) | ||
| .padding(.vertical, VSpacing.md) | ||
|
|
||
| Spacer(minLength: 0) |
There was a problem hiding this comment.
🚩 HStack+Spacer width proposal difference for long wrapping text
The replacement of .frame(maxWidth: .infinity, alignment: .leading) with HStack(spacing: 0) { Content; Spacer(minLength: 0) } has a subtle difference in how SwiftUI distributes width among flexible children. With .frame(maxWidth: .infinity), the content view was the sole flexible child and received all remaining width. With the Spacer pattern, the content view theoretically competes with the Spacer for space. In practice, SwiftUI's layout algorithm sizes less-flexible children first and Spacer absorbs remaining space, so for most content (especially text that wraps), the behavior is equivalent. However, for InlineChatErrorAlert.swift:178 specifically, the Spacer(minLength: 0) sits alongside a padded VStack containing error messages. If an error message's ideal single-line width is very large, the VStack could receive a narrower proposal than before, causing slightly different text wrapping. This is worth a quick visual check on long error messages with debug details to confirm the card still looks correct.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Not a regression — the VStack's text children have .fixedSize(horizontal: false, vertical: true), which means they accept any proposed width and wrap to fit. HStack proposes the full remaining width (after the 3pt accent bar) to the VStack first because it's the less-flexible child (it has content-dependent sizing), then the Spacer(minLength: 0) absorbs whatever space the VStack doesn't claim. In practice, wrapping text always claims the full proposed width, so the Spacer gets 0pt — identical to the previous .frame(maxWidth: .infinity) behavior.
Worth confirming visually with long error messages in Xcode (flagged in the human review checklist), but not a functional regression.
…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.
Eliminates
_FlexFrameLayoutsources inside LazyVStack message cell views that cause recursiveexplicitAlignmentcascades during scroll — each.frame(maxWidth: .infinity)triggers O(depth) alignment queries per cell, multiplied across all visible cells duringLazyStack.measureEstimates, contributing to multi-second main-thread hangs (5s stall observed in hang diagnostics).Why needed:
.frame(maxWidth: .infinity, alignment: .leading)compiles to_FlexFrameLayout, which recursively callssizeThatFitson children to determine their size before applying the frame. Inside LazyVStack cells, this recursive measurement propagates through deeply nested VStack/HStack hierarchies viaexplicitAlignmentqueries. The safe replacement —HStack { content; Spacer(minLength: 0) }— achieves the same leading-aligned layout without triggering child measurement, because HStack usesplaceSubviews(O(1) per child) instead of recursivesizeThatFits.Why safe: Each replacement is a mechanical swap that preserves identical visual output — text remains leading-aligned, ScrollView heights remain definite, and background/clip shapes remain unchanged. The
InlineVideoAttachmentViewrestructuring (ZStack → overlay) is also layout-equivalent: the save button was the only child that needed.topTrailingpositioning, and.overlay(alignment:)provides exactly that without forcing all siblings into a non-center alignment. CI skips macOS builds, so local Xcode verification is required — see human review checklist below.Alternatives considered and rejected:
.frame(width: containerWidth)via GeometryReader — would require threading a container width through the view hierarchy or adding GeometryReader, adding complexity.HStack + Spacerachieves the same result with zero new state..frame(maxWidth: .infinity)without alignment parameter — still creates_FlexFrameLayoutwhich queriesexplicitAlignmenton children (defaults to.center), triggering the same recursive cascade. Not safe inside LazyVStack cells..frame(maxWidth: .infinity, maxHeight: .infinity)from InlineVideoAttachmentView subviews without restructuring — would break layout because the original ZStack used.topTrailingalignment, causing VStack-based content to shrink to intrinsic size and position at top-right. TheZStack(.center) + .overlay(alignment: .topTrailing)restructuring avoids this regression..frame(maxWidth: .infinity, maxHeight: .infinity)back to InlineVideoAttachmentView subviews — would reintroduce FlexFrame inside the cell hierarchy. Instead,Color.clear(a greedy view) inserted into each subview's ZStack fills the parent-proposed size without FlexFrame measurement..frame(maxWidth: 360)to.frame(width: 360)on audio/video cards — would clip content when the chat pane is narrower than 360pt (PanelCoordinator allows ~300pt minimum). BoundedmaxWidth: 360is intentionally preserved because it's not the problematic.infinitypattern — child measurement is bounded to 360pt, not unbounded.References:
explicitAlignment— alignment queries propagate recursively when children don't provide explicit valuessizeThatFits/placeSubviewscontractHuman review checklist
Since CI cannot build macOS targets, a reviewer should verify in Xcode:
Color.cleargreedy-fill approach replaces the old.frame(maxWidth: .infinity, maxHeight: .infinity)).overlay(alignment: .topTrailing)restructuring.frame(maxWidth: .infinity, maxHeight: .infinity)—VColor.auxBlack.opacity(0.8)is a Color (greedy by default), but worth confirmingPrompt / plan
Fix
CodePreviewViewFlexFrame anti-pattern atChatWidgetViews.swift:114,122and audit for any other remaining.frame(maxWidth:)instances inside LazyVStack cells. Replace withHStack { content; Spacer() }or equivalent non-FlexFrame pattern, consistent with PR #25844.Test plan
.frame(maxWidth: .infinity)inside LazyVStack cell hierarchy — none remain after this PRLink to Devin session: https://app.devin.ai/sessions/fdedead3e59c410389996f4672776d6d
Requested by: @ashleeradka