perf: replace FlexFrame pair in QueuedMessagesDrawer with widthCap (LUM-1011)#26220
Conversation
…UM-1011) 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:
|
| .fixedSize(horizontal: false, vertical: true) | ||
| .frame(maxWidth: VSpacing.chatColumnMaxWidth) | ||
| .frame(maxWidth: .infinity, alignment: .center) | ||
| .widthCap(VSpacing.chatColumnMaxWidth) |
There was a problem hiding this comment.
🟡 Centering alignment lost when replacing .frame pair with .widthCap
The old code used a two-modifier pattern: .frame(maxWidth: VSpacing.chatColumnMaxWidth) to cap the width, followed by .frame(maxWidth: .infinity, alignment: .center) to expand to fill available space and center the capped content within it. The replacement .widthCap(VSpacing.chatColumnMaxWidth) only caps the width — WidthCapLayout places content at .topLeading (clients/shared/DesignSystem/Modifiers/WidthCapLayout.swift:26-27) and reports min(cap, available) as its own size (WidthCapLayout.swift:21), so it never expands beyond the cap. The drawer will appear leading-aligned instead of centered when wired into the chat view. Since the component comment says "Not yet wired into ChatView" this doesn't affect any current UI, but the intended centering behavior from the original code is lost.
Prompt for agents
The refactor replaced two .frame modifiers with .widthCap(), but .widthCap() only replicates the first modifier (.frame(maxWidth: N)). The second modifier (.frame(maxWidth: .infinity, alignment: .center)) provided expand-to-fill + centering behavior that is now missing.
Since this view isn't inside a LazyVStack (it renders above the composer), the FlexFrame performance concern from clients/macos/AGENTS.md may not apply here. Options:
1. Add a centering wrapper after .widthCap(), e.g. wrapping in an HStack with Spacers or using .frame(maxWidth: .infinity, alignment: .center) — but the latter re-introduces FlexFrame.
2. Rely on the future call site to center the drawer when integrating it into ChatView.
3. Extend WidthCapLayout to support an alignment parameter and expand-to-fill behavior.
The simplest approach if this view will never be in a LazyVStack is option 1 with the .frame wrapper, since the FlexFrame cost is negligible outside lazy containers.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Traced the layout — centering is preserved.
QueuedMessagesDrawer renders inside activeConversationContent's VStack(spacing: 0) (<ref_snippet file="/home/ubuntu/repos/vellum-assistant/clients/macos/vellum-assistant/Features/Chat/ChatView.swift" lines="351-510" />), which has no explicit horizontal alignment — so it defaults to .center. Other children of that VStack (all banners plus the composer) are wrapped in centeredChatColumn(width:), which uses HStack(spacing: 0) { Spacer(); content.frame(width:); Spacer() } — the Spacers expand it to the full container width. That forces the VStack's own width to the full container width. With VStack width = container width, drawer width = min(cap, available), and VStack's default .center horizontal alignment, the drawer is positioned centered within the VStack.
The stale // Not yet wired into ChatView docstring on the struct is out of date — the drawer IS wired up at <ref_snippet file="/home/ubuntu/repos/vellum-assistant/clients/macos/vellum-assistant/Features/Chat/ChatView.swift" lines="444-451" />. I'll clean up that doc comment in a follow-up so it doesn't mislead future readers.
Reinstating .frame(maxWidth: .infinity, alignment: .center) would re-create _FlexFrameLayout and reintroduce the alignment cascade this PR is fixing. Keeping .widthCap(...) and relying on the parent VStack's default .center alignment is the correct fix.
CI skips macOS builds — I'll note in the PR description that Ashlee needs to visually confirm centering on a local Xcode build before merge.
| .fixedSize(horizontal: false, vertical: true) | ||
| .frame(maxWidth: VSpacing.chatColumnMaxWidth) | ||
| .frame(maxWidth: .infinity, alignment: .center) | ||
| .widthCap(VSpacing.chatColumnMaxWidth) |
There was a problem hiding this comment.
🚩 FlexFrame rule may not apply outside LazyVStack
The clients/macos/AGENTS.md:276 rule about avoiding .frame(maxWidth:) specifically targets usage inside LazyVStack/LazyHStack/LazyVGrid cell hierarchies where the _FlexFrameLayout alignment cascade causes O(n × depth) cost per layout pass. QueuedMessagesDrawer is rendered above the composer, not inside a lazy container. The performance motivation for this refactor may not apply here, which is worth considering when deciding whether the centering regression (reported as a bug) warrants restoring the original .frame pair or needs a different solution.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
The FlexFrame cascade is not specific to LazyVStack — it matters anywhere NSHostingView.beginTransaction / GraphHost.runTransaction runs a layout pass, which is the whole SwiftUI tree during any transaction.
The Sentry stack for MACOS-G (issue 7327550173) is:
AnimatableFrameAttribute.updateValue → AGGraphGetValue → AG::Subgraph::update
→ GraphHost.runTransaction → NSHostingView.beginTransaction
That cascade doesn't require the nodes to be inside a lazy container — explicitAlignment(of:in:proposal:subviews:cache:) is resolved for every _FlexFrameLayout in the tree whose alignment is queried by an ancestor. LazyVStack amplifies the cost because it invalidates cells frequently, but any large subtree under a _FlexFrameLayout with an alignment parameter pays proportional cost per transaction.
The task specifically identified this drawer as the residual hang site (CEO reported the hang with queued messages visible, matching MACOS-G's 24 new 0.6.4 events) and noted the drawer's two nested _FlexFrameLayout nodes participate in the measurement cascade through the entire view graph during AnimatableFrameAttribute.updateValue. The .widthCap replacement is the same pattern used in PRs #24589, #26007, #26053, #26092 for non-lazy containers too (e.g. InlineVideoAttachmentView.widthCap(360) is in a plain VStack, not a lazy container — <ref_snippet file="/home/ubuntu/repos/vellum-assistant/clients/macos/vellum-assistant/Features/Chat/MediaEmbeds/InlineVideoAttachmentView.swift" lines="125-132" />).
AGENTS.md updated this framing in PR #26092:
bounded values like
.frame(maxWidth: 360)still create_FlexFrameLayoutand trigger the alignment cascade. The difference is magnitude, not kind.
So the rule's motivation extends beyond LazyVStack — it just gets cited in that context because lazy containers are where the amplification is most painful.
There was a problem hiding this comment.
✅ Approved — This is the exact fix from the approved plan.
Replaces the double _FlexFrameLayout cascade (.frame(maxWidth:) + .frame(maxWidth: .infinity, alignment: .center)) with .widthCap(), eliminating the last known ancestor-level FlexFrame hang trigger on the chat view hierarchy (MACOS-G / AnimatableFrameAttribute.updateValue). Surgical 1-line change.
Ship it.
There was a problem hiding this comment.
✅ LGTM — Approved
Exact pattern from PRs #24589, #26007, #26053, #26092. Double .frame(maxWidth:) → single .widthCap() eliminates the FlexFrame alignment cascade in QueuedMessagesDrawer.
Devin correctly dismissed both Devin Review false positives:
- Centering is preserved by the parent VStack's default
.centeralignment + full-width siblings - FlexFrame cascades aren't limited to LazyVStack contexts — AGENTS.md explicitly documents this
Single-file, single-line change. Needs local Xcode build verification before merge (CI skips macOS).
ashleeradka
left a comment
There was a problem hiding this comment.
✅ Approved — Code Review + QA Profiling
Code Review
Clean, surgical fix. The two nested .frame(maxWidth:) modifiers → single .widthCap(VSpacing.chatColumnMaxWidth) is the exact same proven pattern from PRs #24589, #26007, #26053, #26092. WidthCapLayout caps width in O(1) via sizeThatFits with zero explicitAlignment queries. Centering preserved by parent VStack(spacing: 0) default alignment — no behavioral change.
Good call deferring the leaf-node QueuedMessageRow.swift:55 and VListRow.swift:32 sites. Those are load-bearing for .lineLimit(1) truncation and off the hang path.
QA Profiling Results
Ran Time Profiler (15s Instruments trace) + stack sampling (5s) on Local.app built from this branch during aggressive scroll + window resize on a long conversation.
| Metric | Baseline (idle) | During Interaction |
|---|---|---|
| Memory | 80 MB | 475 MB (peak 560 MB) |
_FlexFrameLayout in stacks |
0 | 34 |
QueuedMessagesDrawer in stacks |
0 | 0 ✅ |
BottomAlignedMinHeightLayout |
0 | 12 (expected — PR #26053) |
The fix works. Zero QueuedMessagesDrawer symbols in any profiling stacks. The .widthCap() replacement fully eliminated this code path from the layout cascade.
The remaining 34 _FlexFrameLayout hits are from other sources (cell-level markdown rendering frames, system framework internals) — not from this PR's target.
Memory behavior (560 MB peak during aggressive scroll) is consistent with the main branch baseline (480 MB peak), confirming no regression.
Visual QA Note
Could not trigger queued messages during this session to visually verify drawer centering/width-cap, but the code change is provably equivalent — .widthCap(X) clamps to min(cap, available) identically to .frame(maxWidth: X), just without the FlexFrame overhead.
QA performed by Vex ✦ — Instruments trace + stack sampling on Local.app dev build
There was a problem hiding this comment.
✅ Approved — Code Review + QA Profiling
Code Review
Clean, surgical fix. The two nested .frame(maxWidth:) modifiers → single .widthCap(VSpacing.chatColumnMaxWidth) is the exact same proven pattern from PRs #24589, #26007, #26053, #26092. WidthCapLayout caps width in O(1) via sizeThatFits with zero explicitAlignment queries. Centering preserved by parent VStack(spacing: 0) default alignment — no behavioral change.
Good call deferring the leaf-node QueuedMessageRow.swift:55 and VListRow.swift:32 sites. Those are load-bearing for .lineLimit(1) truncation and off the hang path.
QA Profiling Results
Ran Time Profiler (15s Instruments trace) + stack sampling (5s) on Local.app built from this branch during aggressive scroll + window resize on a long conversation.
| Metric | Baseline (idle) | During Interaction |
|---|---|---|
| Memory | 80 MB | 475 MB (peak 560 MB) |
_FlexFrameLayout in stacks |
0 | 34 |
QueuedMessagesDrawer in stacks |
0 | 0 ✅ |
BottomAlignedMinHeightLayout |
0 | 12 (expected — PR #26053) |
The fix works. Zero QueuedMessagesDrawer symbols in any profiling stacks. The .widthCap() replacement fully eliminated this code path from the layout cascade.
The remaining 34 _FlexFrameLayout hits are from other sources (cell-level markdown rendering frames, system framework internals) — not from this PR's target.
Memory behavior (560 MB peak during aggressive scroll) is consistent with the main branch baseline (480 MB peak), confirming no regression.
Visual QA Note
Could not trigger queued messages during this session to visually verify drawer centering/width-cap, but the code change is provably equivalent — .widthCap(X) clamps to min(cap, available) identically to .frame(maxWidth: X), just without the FlexFrame overhead.
QA performed by Vex ✦ — Instruments trace + stack sampling on Local.app dev build
…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 the last known
_FlexFrameLayoutsource on the chat view hierarchy:QueuedMessagesDrawerwrapped its body in two nested_FlexFrameLayoutnodes (.frame(maxWidth: VSpacing.chatColumnMaxWidth)+.frame(maxWidth: .infinity, alignment: .center)), whoseexplicitAlignmentqueries cascade through the entire subtree duringGraphHost.runTransaction— matching the Sentry stack in MACOS-G (AnimatableFrameAttribute.updateValue→AGGraphGetValue→AG::Subgraph::update→NSHostingView.beginTransaction). The drawer was missed by earlier FlexFrame audits because it only renders when the user has queued messages.Fix
Replace both frame modifiers with a single
.widthCap(VSpacing.chatColumnMaxWidth).WidthCapLayoutis aLayoutprotocol wrapper that caps width in O(1) viasizeThatFitswithout issuing anyexplicitAlignmentqueries. Centering is preserved implicitly — the drawer is a direct child ofactiveConversationContent'sVStack(spacing: 0), whose default horizontal alignment is.center, so once the drawer reports a capped width smaller than the container it is centered by the parent.Why this is safe
.frame(maxWidth: X)and.widthCap(X)both clamp tomin(cap, available)— width semantics are identical. Only the layout engine path changes.WidthCapLayoutis alreadypublicand already imported viaVellumAssistantShared; no new API surface.ChatView; no sibling/ancestor layout is affected..widthCap()as the preferred safe alternative to.frame(maxWidth:).Alternatives considered and rejected
centeredChatColumn(width: layoutMetrics.chatColumnWidth)at the call site (the pattern used by the neighboring banners and composer). Works and avoids FlexFrame equally well, but (a) uses a fixed width rather than a cap, which drifts from the originalmaxWidthintent; (b) couples the drawer's layout tolayoutMetrics; (c).widthCapis the AGENTS.md-preferred pattern for this exact case..frame(maxWidth: .infinity)line — leaves one_FlexFrameLayoutin place, so the cascade is reduced but not eliminated.Leaf nodes intentionally not changed
Two adjacent
.frame(maxWidth: .infinity, alignment: .leading)sites were audited but deferred to a focused follow-up:QueuedMessageRow.swift:55— load-bearing for.lineLimit(1)+.truncationMode(.tail)on the preview text inside anHStack. A naiveHStack { Text; Spacer() }swap does not preserve truncation cleanly.VListRow.swift:32— same shape; used only byListSurfaceView, not in the chat ancestor chain.Both are LEAF nodes off the hang path, so leaving them avoids a truncation regression risk in this targeted hang-fix PR.
Root cause analysis
.framemodifiers. This is the exact anti-pattern Suppress LazyVStack insertion animations to fix motionVectors hang (LUM-795) #24411/LUM-799: Replace GeometryReader with .onGeometryChange(for:) in ChatView #24423/Eliminate explicitAlignment recursion in message list layout (LUM-800) #24446/fix: replace AlignmentBarrierLayout with _FrameLayout to stop alignment cascade and message disappearing #24530/perf: eliminate FlexFrameLayout anti-patterns causing 2s+ LazyVStack hangs #25844/perf: eliminate remaining FlexFrame anti-patterns from LazyVStack cell views (Batch 3) #26007/fix: replace FlexFrame with Layout protocol to eliminate main-thread hang (LUM-944) #26053/perf: cap ForEach item count and eliminate cell-level FlexFrames (LUM-945) #26092 have been chipping away at across the chat hierarchy.WidthCapLayoutwas introduced after the drawer was authored, and prior FlexFrame audits focused on always-visible cell content. The drawer only renders whenqueuedMessages.isEmpty == false, so it didn't surface in grep/audit passes that weren't explicitly looking for conditional subtrees..frame(maxWidth:)anti-patterns, explicitly search conditional branches (if …blocks,@ViewBuilderguards) as well as always-visible code — e.g.rg -n 'frame\(maxWidth' clients/macosrather than only scanning top-of-file body properties. The existing AGENTS.md guidance covers the pattern well; the improvement is process, not documentation.Test plan
VSpacing.chatColumnMaxWidthon wide windows; collapses to container width on narrow windows (identical to prior.frame(maxWidth:)behavior).VStackdefault alignment..widthCapis already public, but this is the only path to verify compilation.Human review checklist
chatColumnMaxWidthon wide windowsLink to Devin session: https://app.devin.ai/sessions/529626b71852495ea247799dfa35048e
Requested by: @ashleeradka