Skip to content

perf: replace FlexFrame pair in QueuedMessagesDrawer with widthCap (LUM-1011) #26220

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

Merged
vex-assistant-bot merged 1 commit into from
Apr 17, 2026
Merged

perf: replace FlexFrame pair in QueuedMessagesDrawer with widthCap (LUM-1011) #26220

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 1 addition & 2 deletions clients/macos/vellum-assistant/Features/Chat/QueuedMessagesDrawer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,7 @@ struct QueuedMessagesDrawer: View {
.strokeBorder(VColor.borderBase, lineWidth: 1)
)
.fixedSize(horizontal: false, vertical: true)
.frame(maxWidth: VSpacing.chatColumnMaxWidth)
.frame(maxWidth: .infinity, alignment: .center)
.widthCap(VSpacing.chatColumnMaxWidth)

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟡 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.
Open in Devin Review

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🚩 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.

Open in Devin Review

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 _FlexFrameLayout and 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.

}

private func header(queuedMessages: [ChatMessage]) -> some View {
Expand Down