perf: eliminate remaining FlexFrame anti-patterns from LazyVStack cell views (Batch 3)#26007
Conversation
…iews (Batch 3) Replace .frame(maxWidth:) with HStack+Spacer and WidthCapLayout across 7 files (14 instances) to eliminate _FlexFrameLayout recursive measurement cascades. - ToolCallChip: 4 instances (3 images + 1 text in ScrollView) - ToolCallProgressBar: 4 instances (outer VStack + 2 images + 1 text) - InlineAppCreatedCard: 1 instance (preview image, parent already constrained) - InlineDynamicPagePreview: 1 instance (content VStack, parent constrained) - InlineTableWidget: 1 instance (resize handle overlay, redundant frame) - ModelListBubble: 1 instance (.frame(maxWidth: 480) -> .widthCap(480)) - CommandListBubble: 1 instance (.frame(maxWidth: 400) -> .widthCap(400)) Extract WidthCapLayout from InlineSurfaceRouter.swift to shared DesignSystem/Modifiers/WidthCapLayout.swift for reuse. 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:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9979b5c5a0
ℹ️ 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".
ashleeradka
left a comment
There was a problem hiding this comment.
Ship it ✦ — Clean mechanical conversion of all 14 remaining .frame(maxWidth:) instances. Approved.
What's good
- WidthCapLayout extraction: Identical code promoted from
fileprivate→internal, shared location atDesignSystem/Modifiers/WidthCapLayout.swift. InlineSurfaceRouter's 5 call sites resolve from the new location. DRY and correct. - ToolCallChip (4 sites): All image previews (macOS canOpenImage, macOS else, iOS) and text result in ScrollView wrapped in
HStack(spacing: 0) { content; Spacer(minLength: 0) }. Matches established pattern from PRs #25844/#25947. - ToolCallProgressBar (4 sites): Outer VStack
.frame(maxWidth:)removed (fills parent naturally). Images and text result wrapped correctly. - InlineAppCreatedCard:
.frame(maxWidth: .infinity)removed from preview image. Parent.widthCap(400)from InlineSurfaceRouter constrains width. - InlineDynamicPagePreview:
.frame(maxWidth: .infinity, alignment: .leading)removed. Parent.widthCap()handles constraint. - InlineTableWidget: Resize handle converted to definite
.frame(width: resizeHandleWidth). Correct. - ModelListBubble / CommandListBubble:
.frame(maxWidth: 480/400)→.widthCap(480/400). Clean. - Alternatives table in PR description is excellent — documents rejected approaches so future work doesn't revisit dead ends.
QA verification needed (CI skips macOS builds)
Devin's own checklist is good — the two I'd prioritize:
- InlineAppCreatedCard preview image:
.fill+.frame(height: 140)with no explicit width. Should fill.widthCap(400)parent, but.fillwithout width constraint could overflow clip bounds. Verify visually. - ToolCallProgressBar width: Outer VStack lost
.frame(maxWidth: .infinity). Should still fill available width from parent, but verify progress bar doesn't shrink when there are few steps.
Sweep result
Zero .frame(maxWidth:) remaining in all 7 target files on this branch. Together with PRs #25844 and #25947, this completes the LazyVStack cell-level FlexFrame cleanup. ✅
— Reviewed by Vex ✦
…Stack+Spacer Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
… (LUM-944) Primary fix: Replace .frame(minHeight:alignment: .bottom) in MessageListView.swift with BottomAlignedMinHeightLayout (Layout protocol). The FlexFrame wraps the entire LazyVStack inside the ScrollView, causing _FlexFrameLayout.explicitAlignment to cascade through every cell — O(n × depth) layout work, 170 spindump samples, 104.96s hang. The Layout-protocol implementation achieves identical visual behavior (pin short content to bottom of viewport in inverted scroll) in O(1) via placeSubviews positioning instead of alignment queries. Secondary fix: Replace .frame(maxWidth:, maxHeight:) in AnimatedImageView.swift with definite dimensions (cgImage path) and widthCap (NSImage fallback) to eliminate cell-level FlexFrames. Follows the same pattern as WidthCapLayout (PR #26007). Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
… (LUM-944) Primary fix: Replace .frame(minHeight:alignment: .bottom) in MessageListView.swift with BottomAlignedMinHeightLayout (Layout protocol). The FlexFrame wraps the entire LazyVStack inside the ScrollView, causing _FlexFrameLayout.explicitAlignment to cascade through every cell — O(n × depth) layout work, 170 spindump samples, 104.96s hang. The Layout-protocol implementation achieves identical visual behavior (pin short content to bottom of viewport in inverted scroll) in O(1) via placeSubviews positioning instead of alignment queries. Secondary fix: Replace .frame(maxWidth:, maxHeight:) in AnimatedImageView.swift with definite dimensions (cgImage path) and widthCap (NSImage fallback) to eliminate cell-level FlexFrames. Follows the same pattern as WidthCapLayout (PR #26007). Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
…hang (LUM-944) (#26053) * fix: replace FlexFrame with Layout protocol to eliminate 104.96s hang (LUM-944) Primary fix: Replace .frame(minHeight:alignment: .bottom) in MessageListView.swift with BottomAlignedMinHeightLayout (Layout protocol). The FlexFrame wraps the entire LazyVStack inside the ScrollView, causing _FlexFrameLayout.explicitAlignment to cascade through every cell — O(n × depth) layout work, 170 spindump samples, 104.96s hang. The Layout-protocol implementation achieves identical visual behavior (pin short content to bottom of viewport in inverted scroll) in O(1) via placeSubviews positioning instead of alignment queries. Secondary fix: Replace .frame(maxWidth:, maxHeight:) in AnimatedImageView.swift with definite dimensions (cgImage path) and widthCap (NSImage fallback) to eliminate cell-level FlexFrames. Follows the same pattern as WidthCapLayout (PR #26007). Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai> * fix: cap both width and height for portrait images in AnimatedImageView Address Devin Review feedback: - CGImage path: use min(maxDimension/width, maxDimension/height, 1.0) to cap both dimensions (same logic as gifSize), fixing portrait images that exceeded maxDimension in height. - NSImage fallback: compute definite frame from image.size with both-axis capping instead of widthCap-only, restoring height constraint. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai> * fix: make BottomAlignedMinHeightLayout public for cross-module access Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai> * fix: use consistent proposal in placeSubviews to prevent layout oscillation In placeSubviews, child.sizeThatFits was called with ProposedViewSize(bounds.width, bounds.height) instead of the same proposal that sizeThatFits received. When minHeight > child height, this proposed the expanded height to the child, which could return a different size — causing SwiftUI to detect a layout inconsistency and re-evaluate layout every frame. Visible as rapid cursor blinking in the conversation input on new conversations. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai> * chore: clean up PR-iteration-specific comments Remove LUM-944 ticket references and iteration-specific language from code comments. Comments now explain the architectural rationale without referencing the PR or its history. 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.
Removes the last 14
.frame(maxWidth:)instances inside LazyVStack cell views across 8 files, eliminating_FlexFrameLayoutnodes whose recursivesizeThatFits/explicitAlignmentcascades cause multi-second main-thread hangs. ExtractsWidthCapLayoutfromInlineSurfaceRouter.swift(fileprivate) toDesignSystem/Modifiers/WidthCapLayout.swift(internal) soModelListBubbleandCommandListBubblecan reuse it for finite-width caps. Continues the pattern from PRs #25844 and #25947.Why needed:
.frame(maxWidth:)compiles to_FlexFrameLayout, which recursively callssizeThatFitson all children to resolve alignment — O(n × depth) inside LazyVStack cells. The safe replacements areHStack { content; Spacer(minLength: 0) }(leading alignment viaplaceSubviews, O(1) per child) andWidthCapLayout(Layout-protocol width cap, O(1)).Why safe: Each replacement is a mechanical swap preserving identical visual output. For
InlineDynamicPagePreview, the VStack is wrapped inHStack(spacing: 0) { VStack { ... }; Spacer(minLength: 0) }with.contentShape(Rectangle())on the HStack — this preserves the full-width tap target that.frame(maxWidth: .infinity)previously provided (just removing the frame would have shrunk the Button's hit area, creating dead zones in the card chrome from.inlineWidgetCard(interactive: true)).Alternatives not taken:
.containerRelativeFrame(.horizontal)GeometryReaderto thread container width.frame(maxWidth: 480/400)as-is_FlexFrameLayout; triggers alignment queries on childrenWidthCapLayoutper fileReferences:
CI skips macOS/iOS builds — local Xcode verification required:
HStack + Spacerwith.contentShape(Rectangle())on the HStack. Verify the entire card remains clickable for short titles/descriptions.frame(maxWidth: .infinity, alignment: .leading). Verify progress bar still fills available width when there are few steps.fill+.frame(height: 140)with no explicit width. Verify image fills card width correctly under.widthCap(400)maxWidth/maxHeight: .infinitytowidth: resizeHandleWidth. Verify column resize dragging still works — height is no longer explicitly set (overlay proposes parent bounds)Link to Devin session: https://app.devin.ai/sessions/85e233c00d6a4c8c99c1f62af7ff5867
Requested by: @ashleeradka