fix: eliminate _FlexFrameLayout anti-pattern from chat cell hierarchy#24589
Conversation
…rapper to fix 35s hang .frame(maxHeight:, alignment:) creates _FlexFrameLayout which recursively measures children and resolves explicitAlignment through the entire LazyVStack subtree — O(n × depth) per layout pass, causing 35s+ main-thread hangs after just 3 messages. Replace with a two-path conditional: - Collapsed: .frame(height:) creates _FrameLayout — O(1), no alignment cascade - Expanded/short: no frame modifier — avoids _FlexFrameLayout entirely GeometryReader moves to only the expanded/short path since the intrinsic height measurement is only needed once per cell lifecycle (@State persists it). Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
🤖 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:
|
![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: 0cf524760a
ℹ️ 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".
| if needsCollapse { | ||
| // Collapsed: definite height — _FrameLayout, O(1), no alignment cascade. | ||
| content() | ||
| .frame(height: userMessageMaxCollapsedHeight, alignment: .top) | ||
| .clipped() |
There was a problem hiding this comment.
This collapsed branch never updates userMessageIntrinsicHeight, so once a message is collapsed the collapsibility decision is based on stale height data. If layout later changes (for example window resize, sidebar width change, or typography changes), content can become shorter than the 150pt threshold but isCollapsible remains true, leaving an unnecessary fixed-height bubble and “Show more” control. The previous code continuously measured intrinsic height even while collapsed, so this is a behavioral regression in userMessageHeightWrapper.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 5f52c4b. Switched to a single-path approach: .frame(height: needsCollapse ? userMessageMaxCollapsedHeight : nil, alignment: .top).
.frame(height:alignment:) uses the definite-frame API which creates _FrameLayout (not _FlexFrameLayout) — O(1), no alignment cascade. When height is nil (expanded/short), _FrameLayout passes through the child's natural height.
The GeometryReader now runs on all paths, so userMessageIntrinsicHeight stays up to date even on window resize while collapsed.
![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.
…nd continuous measurement Addresses review feedback: - Codex: GeometryReader now runs on all paths, keeping userMessageIntrinsicHeight up to date even while collapsed (e.g. on window resize) - Devin Review: Single view identity preserved (no _ConditionalContent), so withAnimation still drives a smooth height transition on expand/collapse .frame(height:, alignment:) uses the definite-frame API which creates _FrameLayout — O(1), no alignment cascade. When height is nil (expanded or short content), _FrameLayout passes through the child's natural height. Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
Remove .frame(maxWidth:) / .frame(maxHeight:) modifiers that create _FlexFrameLayout (O(n × depth) alignment cascade) inside LazyVStack cells. These are the highest-impact remaining instances after the ChatBubble fix. MarkdownRenderer.swift: delete 6 redundant .frame(maxWidth: .infinity) — parent VStack(alignment: .leading) already proposes full width. ToolConfirmationBubble.swift: delete 5 redundant .frame(maxWidth: .infinity) — parent VStacks + backgrounds handle sizing. InlineTableWidget.swift: delete 7 instances, convert 1 maxWidth to definite .frame(width:) — TableColumnLayout.placeSubviews proposes explicit column widths so cells still fill correctly. InlineSurfaceRouter.swift: replace 6 instances with WidthCapLayout (custom Layout, O(1)) for finite caps and delete the .infinity path entirely. WidthCapLayout caps proposed width without measuring children first, avoiding the FlexFrame alignment cascade. InlineImageEmbedView.swift: replace double FlexFrame (maxWidth + maxHeight) with GeometryReader + PreferenceKey + ternary .frame(height:) pattern; delete redundant .frame(maxWidth: .infinity) from placeholder. Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
devin-ai-integration
Bot
changed the title
dvargasfuertes
deleted the
devin/1775761669-fix-flexframelayout-hang-user-messages
branch
There was a problem hiding this comment.
🚩 Removed .frame(maxWidth: .infinity) from ToolConfirmationBubble may affect card fill behavior
Several .frame(maxWidth: .infinity, alignment: .leading) modifiers were removed from pendingContent, commandExplanationBanner, codePreviewBlock, and the diff view. These views render inside a tool confirmation card that has .background(RoundedRectangle(...)). Without .frame(maxWidth: .infinity), the VStack/HStack containers will be content-width rather than filling the available width. The card background fills the container's size, so the background may not span the full chat bubble width as before. However, the pendingContent VStack contains children that naturally span the proposed width: the confirmationDescription Text with .fixedSize(horizontal: false, vertical: true) wraps at the proposed width, and the button HStack with Spacer(minLength: VSpacing.md) fills the width. So in practice the card should still appear full-width. The commandExplanationBanner HStack contains a VStack with flexible Text elements that accept the proposed width. Worth verifying visually that the card and banner backgrounds fill correctly.
Was this helpful? React with 👍 or 👎 to provide feedback.
| } | ||
| ) | ||
| .onPreferenceChange(ImageHeightKey.self) { imageIntrinsicHeight = $0 } | ||
| .frame(height: imageIntrinsicHeight > 300 ? 300 : nil) |
There was a problem hiding this comment.
🔴 Strict > comparison creates infinite layout oscillation for images taller than 300pt
The height-capping logic in InlineImageEmbedView uses .frame(height: imageIntrinsicHeight > 300 ? 300 : nil) with a GeometryReader that measures the content's rendered height. This creates a feedback loop for any image whose natural height exceeds 300pt:
imageIntrinsicHeight = 0→frame(height: nil)→ image renders at natural height (e.g., 500) → GeometryReader reports 500imageIntrinsicHeight = 500→500 > 300→frame(height: 300)→ image constrained to 300 → GeometryReader reports 300imageIntrinsicHeight = 300→300 > 300is false →frame(height: nil)→ image renders at 500 again → GeometryReader reports 500- Back to step 2 — infinite oscillation
The > operator causes the value 300 to flip the constraint off, re-expanding the image, which re-measures above 300, re-enabling the constraint, ad infinitum. SwiftUI's loop detection may cap the number of re-renders but will log runtime warnings, waste CPU, and produce unpredictable visual height. Using >= stabilizes at step 3: 300 >= 300 → frame(height: 300) → reports 300 → no change.
| .frame(height: imageIntrinsicHeight > 300 ? 300 : nil) | |
| .frame(height: imageIntrinsicHeight >= 300 ? 300 : nil) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| } | ||
| .scrollDisabled(isResizingColumn) | ||
| .frame(maxWidth: maxTableViewportWidth, alignment: .leading) | ||
| .frame(width: maxTableViewportWidth) |
There was a problem hiding this comment.
🚩 Fixed-width horizontal scroll viewport won't shrink on very narrow windows
The change from .frame(maxWidth: maxTableViewportWidth, alignment: .leading) to .frame(width: maxTableViewportWidth) means the horizontal scroll container is always exactly 728pt wide (760 - 2*16). The old maxWidth allowed the container to shrink when the parent proposed less. With the main window's minWidth: 800 (clients/macos/vellum-assistant/Features/MainWindow/MainWindowView.swift:590), and sidebar potentially consuming 200+ pixels, the chat area could theoretically be narrower than 728pt when the sidebar is expanded. In that scenario, the table would overflow its parent. This is mitigated by the fact that chatBubbleMaxWidth (760) already assumes the full chat content width, and the outer widthCap(standardWidgetMaxWidth) would propose a smaller width — but the inner .frame(width:) ignores the proposal. This is consistent with the macOS AGENTS.md rule to prefer .frame(width:) over .frame(maxWidth:) inside LazyVStack cells, though using a computed min(containerWidth, maxTableViewportWidth) would be more robust.
Was this helpful? React with 👍 or 👎 to provide feedback.
Adds CI-enforced guard tests that scan Swift source files for three known LazyVStack performance anti-patterns: 1. FlexFrameLayout (.frame(maxWidth:) / .frame(maxHeight:)) in cell hierarchy 2. motionVectors transitions (.transition(.move(edge:))) in cell hierarchy 3. withAnimation in scroll handlers (motionVectors cascade) Prevents regression of fixes from PRs #24321, #24375, #24411, #24446, #24530, #24570, #24589. Part of #24613. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* test: add SwiftUI performance guard tests for LazyVStack anti-patterns Adds CI-enforced guard tests that scan Swift source files for three known LazyVStack performance anti-patterns: 1. FlexFrameLayout (.frame(maxWidth:) / .frame(maxHeight:)) in cell hierarchy 2. motionVectors transitions (.transition(.move(edge:))) in cell hierarchy 3. withAnimation in scroll handlers (motionVectors cascade) Prevents regression of fixes from PRs #24321, #24375, #24411, #24446, #24530, #24570, #24589. Part of #24613. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: add missing cell files to LAZY_VSTACK_CELL_FILES and remove PR references Adds the 10 allowlisted file basenames to the cell hierarchy list so the allowlist is actually consulted. Removes historical PR numbers from the file header comment per AGENTS.md guidance. Co-Authored-By: Claude <noreply@anthropic.com> * fix: use grep -E for portable ERE alternation in FlexFrame guard BSD grep (macOS default) doesn't support \| in BRE mode. Switch to grep -E with | for alternation so the guard works for local dev too. Co-Authored-By: Claude <noreply@anthropic.com> * fix: add SubagentEventsReader to cell files and escape dots in transition grep Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ed vertical stretching The .frame(maxWidth: .infinity, maxHeight: .infinity) modifier caused the document preview card to expand to fill all available vertical space proposed by the parent chat cell during document writing/streaming. Remove the entire frame modifier: - maxHeight: .infinity was the direct cause of the stretching bug - maxWidth: .infinity was redundant (HStack already contains a Spacer()) - Both created _FlexFrameLayout, the anti-pattern PR #24589 eliminated from 5 other files but missed here Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
…ed vertical stretching (#25643) The .frame(maxWidth: .infinity, maxHeight: .infinity) modifier caused the document preview card to expand to fill all available vertical space proposed by the parent chat cell during document writing/streaming. Remove the entire frame modifier: - maxHeight: .infinity was the direct cause of the stretching bug - maxWidth: .infinity was redundant (HStack already contains a Spacer()) - Both created _FlexFrameLayout, the anti-pattern PR #24589 eliminated from 5 other files but missed here Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Jason Zhou <jasonczhou3@gmail.com>
…ed vertical stretching (#25643) The .frame(maxWidth: .infinity, maxHeight: .infinity) modifier caused the document preview card to expand to fill all available vertical space proposed by the parent chat cell during document writing/streaming. Remove the entire frame modifier: - maxHeight: .infinity was the direct cause of the stretching bug - maxWidth: .infinity was redundant (HStack already contains a Spacer()) - Both created _FlexFrameLayout, the anti-pattern PR #24589 eliminated from 5 other files but missed here Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Jason Zhou <jasonczhou3@gmail.com>
…UM-1108) Thread content visibly twitches/shifts left-to-right on initial load when switching to a thread containing a collapsible user message. The cause is a geometry-observation feedback loop in userMessageHeightWrapper: - .frame(height: 150) hard-proposes 150pt to its child (_FrameLayout). - onGeometryChange on the same subtree writes the clamped height back into @State userMessageIntrinsicHeight. - On thread switch, .id(conversationId) destroys the ScrollView, wiping @State. The first frame uses the NSString.boundingRect estimate as a fallback; subsequent frames re-observe the clamped height. The disagreement between the two drives the isCollapsible decision across two renders, flipping whether RoundedRectangle(.fill(surfaceLift)) background is applied. That background appearing/disappearing as container width resolves is the visible twitch. This is the SwiftUI anti-pattern Apple warns about in the onGeometryChange docs: geometry observations should not drive state that changes the observed layout. Fix: drive isCollapsible from a deterministic estimate of text + attachment heights, computed from the model. No geometry observation, no @State, no feedback loop. .frame(height:) stays (needed to avoid the _FlexFrameLayout hang from PR #24589). Worst case if the estimate under-shoots: no Show more button and content renders at natural height — identical to existing non-collapsible user messages. No clipping regression. Estimate covers text height (VFont.nsChat = 16pt DM Sans, matching MarkdownSegmentView), per-attachment heights (single image ~200pt, grid tiles 120pt, video ~200pt, audio ~80pt, file chip ~40pt), bubble chrome vertical padding (2 * VSpacing.md), and inter-section VStack spacing. Also fixes the Show less button disappearing on click (same root cause, originally diagnosed in closed PR #26131). Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…geometry (LUM-1108) Thread content visibly twitches/shifts left-to-right on initial load when switching to a thread containing a collapsible user message. The cause is a geometry-observation feedback loop in userMessageHeightWrapper: - .frame(height: 150) hard-proposes 150pt to its child (_FrameLayout). - onGeometryChange on the same subtree writes the clamped height back into @State userMessageIntrinsicHeight. - On thread switch, .id(conversationId) destroys the ScrollView, wiping @State. The first frame uses the NSString.boundingRect estimate as a fallback; subsequent frames re-observe the clamped height. The disagreement between the two drives the isCollapsible decision across two renders, flipping whether RoundedRectangle(.fill(surfaceLift)) background is applied. That background appearing/disappearing as container width resolves is the visible twitch. This is the SwiftUI anti-pattern Apple warns about in the onGeometryChange docs: geometry observations should not drive state that changes the observed layout. Fix: drive isCollapsible from a deterministic estimate of text + attachment heights, computed from the model. No geometry observation, no @State, no feedback loop. .frame(height:) stays (needed to avoid the _FlexFrameLayout hang from PR #24589). Worst case if the estimate under-shoots: no Show more button and content renders at natural height — identical to existing non-collapsible user messages. No clipping regression. Estimate covers text height (VFont.nsChat = 16pt DM Sans, matching MarkdownSegmentView), per-attachment heights (single image ~200pt, grid tiles 120pt, video ~200pt, audio ~80pt, file chip ~40pt), bubble chrome vertical padding (2 * VSpacing.md), and inter-section VStack spacing. Also fixes the Show less button disappearing on click (same root cause, originally diagnosed in closed PR #26131). Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…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.
2 participants
Summary
Fixes a 35s+ main-thread hang after sending ~3 user messages and eliminates the
_FlexFrameLayoutanti-pattern from the 5 highest-impact shared chat files rendered inside LazyVStack cells..frame(maxWidth:)/.frame(maxHeight:)creates_FlexFrameLayoutwhich recursively measures children and resolvesexplicitAlignmentthrough the entire LazyVStack subtree — O(n × depth) per layout pass..frame(width:)/.frame(height:)creates_FrameLayout— O(1), no alignment cascade. This is the same class of issue fixed in #24091, #24321, #24446, and #24563.ChatBubble.swift (critical hang fix)
Replaces
.frame(maxHeight:, alignment:)with.frame(height:, alignment:)inuserMessageHeightWrapper(introduced in #24003, missed by prior cleanup). Uses a single view path (noif/elsebranching) so SwiftUI view identity is preserved andwithAnimationstill drives smooth height transitions. GeometryReader runs on all paths, keepinguserMessageIntrinsicHeightup to date even while collapsed.MarkdownRenderer.swift (6 deletions)
All
.frame(maxWidth: .infinity, alignment: .leading)modifiers are redundant — parentVStack(alignment: .leading)already proposes full width.ToolConfirmationBubble.swift (5 deletions)
Same rationale — parent VStacks and backgrounds handle sizing.
adaptiveScrollFramealready uses safe.frame(height:).InlineTableWidget.swift (7 deletions + 1 width change)
Removes 7 redundant
.frame(maxWidth:)instances. ConvertshorizontalScrollableTablefrom.frame(maxWidth: maxTableViewportWidth)to.frame(width: maxTableViewportWidth)— a definite frame (O(1)) sinceTableColumnLayout.placeSubviewsalready proposes explicit column widths.InlineSurfaceRouter.swift (custom WidthCapLayout + 6 replacements)
Introduces
WidthCapLayout, a privateLayoutconformance that caps proposed width at a maximum without measuring children first — O(1) replacement for.frame(maxWidth:). Deletes the.frame(maxWidth: .infinity)path entirely and replaces finite caps (540, 400, 350) with.widthCap().InlineImageEmbedView.swift (GeometryReader pattern + 1 deletion)
Replaces double FlexFrame (
.frame(maxWidth: .infinity, maxHeight: 300)) with GeometryReader +ImageHeightKeyPreferenceKey +.frame(height:)— same pattern as the ChatBubble fix. Removes redundant.frame(maxWidth: .infinity)from placeholder skeleton.Review & Testing Checklist for Human
Notes
WidthCapLayoutis scopedprivatetoInlineSurfaceRouter.swift. It uses the Layout protocol to cap proposed width without triggering_FlexFrameLayout's recursive measurement. The.widthCap(nil)path is a no-op passthrough.InlineChatErrorAlertandMessageInspectorViewalso use.frame(maxHeight:)but are not inside LazyVStack cells, so they are unaffected.ImageHeightKeyPreferenceKey starts atdefaultValue: 0, so on first renderimageIntrinsicHeight > 300is false and.frame(height: nil)passes through — the image renders at natural height, then GeometryReader measures and caps if needed. Verify there is no visible flash for tall images.Link to Devin session: https://app.devin.ai/sessions/bca2b6aae31f43afb41df0dae571ae1d
Requested by: @Jasonnnz