Skip to content

perf(chat): use FixedWidthLayout for centeredChatColumn (LUM-1276) #29231

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 2 commits into from
May 2, 2026
Merged

perf(chat): use FixedWidthLayout for centeredChatColumn (LUM-1276) #29231

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5277d30
perf(chat): replace .frame(width:) with .fixedWidth() in centeredChat…
devin-ai-integration[bot] May 2, 2026
7cfc73e
docs: soften centeredChatColumn docstring (drop unverified _FrameLayo…
devin-ai-integration[bot] May 2, 2026
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
20 changes: 16 additions & 4 deletions clients/macos/vellum-assistant/Features/Chat/ChatView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,17 +529,29 @@ struct ChatView: View {
)
}

/// Centers chat chrome to the same fixed transcript width using _FrameLayout
/// rather than nested max-width flex frames.
/// Centers chat chrome at the chat-column width using `FixedWidthLayout`.
///
/// `FixedWidthLayout` returns `nil` from `explicitAlignment` and places its
/// child via a `UnitPoint` anchor rather than alignment guides, so it acts
/// as a barrier to parent-initiated alignment queries on the subtree. The
/// inner `HStack { Spacer; content; Spacer }` reproduces the horizontal
/// `.center` positioning that `.frame(width:)` applied to non-filling
/// content; flexible content (anything with `.frame(maxWidth: .infinity)`
/// or an internal `Spacer`) collapses the inner spacers to zero and
/// occupies the full column width.
@ViewBuilder
private func centeredChatColumn<Content: View>(
width: CGFloat,
@ViewBuilder content: () -> Content
) -> some View {
HStack(spacing: 0) {
Spacer(minLength: 0)
content()
.frame(width: width)
HStack(spacing: 0) {
Spacer(minLength: 0)
content()
Spacer(minLength: 0)
}
.fixedWidth(width)
Comment on lines +549 to +554
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot May 2, 2026

Choose a reason for hiding this comment

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

🔴 centeredChatColumn inner HStack+Spacer wrapping causes flexible content to compete with spacers instead of filling full width

The old centeredChatColumn applied .frame(width: width) directly to content, which unconditionally constrained the content to exactly width pixels — both for fixed-size content (centered via default .center alignment) and for flexible content (proposed width and filling it). The new implementation wraps content in HStack(spacing: 0) { Spacer(minLength: 0); content(); Spacer(minLength: 0) }.fixedWidth(width), which proposes width to the inner HStack. The inner HStack then distributes that width among three children: two Spacer(minLength: 0) and the content. For fixed-size content this correctly centers it (spacers expand equally). However, for flexible content — views that expand to fill available space — the content now competes with the two spacers for surplus space rather than being directly constrained to width.

Multiple callers pass flexible content:

  • CreditsExhaustedBanner uses .frame(maxWidth: .infinity, alignment: .leading) (ChatErrorToastView.swift:223)
  • DiskPressureBanner has Spacer(minLength:) in its HStack (ChatErrorToastView.swift:263)
  • MissingApiKeyBanner uses .frame(maxWidth: .infinity) (ChatErrorToastView.swift:405) and Spacer() (ChatErrorToastView.swift:384)
  • ComposerSection → ComposerView uses .frame(maxWidth: .infinity) (ComposerView.swift:332)
  • The read-only label passes HStack { Spacer; Icon; Text; Spacer } (ChatView.swift:436)

The docstring claims flexible content "collapses the inner spacers to zero and occupies the full column width," but this depends on undocumented SwiftUI HStack surplus-distribution behavior. If the surplus is shared equally among all flexible children (which is the standard understanding of HStack layout), banners and the composer would render narrower than the intended column width — a visible layout regression.

Prompt for agents 
The centeredChatColumn function replaced `.frame(width: width)` with `HStack { Spacer; content; Spacer }.fixedWidth(width)`. The intent was to reproduce centering while using FixedWidthLayout instead of _FrameLayout to avoid the alignment cascade. However, the inner HStack forces flexible content (banners with .frame(maxWidth: .infinity), composer, read-only label with Spacers) to compete with the wrapper Spacers for surplus space, instead of being directly proposed `width`.

The simplest correct fix is to make FixedWidthLayout support centered placement natively (e.g. add an alignment parameter that defaults to .center), so content can be directly wrapped with .fixedWidth(width) without needing surrounding Spacers. Alternatively, apply .fixedWidth(width) directly to content() and accept topLeading placement (the outer HStack { Spacer; ...; Spacer } already handles page-level centering), since all current callers either have internal centering (read-only label) or want full-width fill (banners, composer).

Key files: FixedWidthLayout.swift (the Layout implementation), ChatView.swift:543-557 (centeredChatColumn), and all callers listed in ChatView.swift lines 368-448 plus composerSection at line 461.
Open in Devin Review

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

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot May 2, 2026

Choose a reason for hiding this comment

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

Confirmed and addressed in follow-up #29232. The PR was merged before the fix landed, so the change is on a separate branch.

You're right that relying on HStack surplus distribution is undocumented. The follow-up adopts the MessageListView.swift:147-182 shape — .fixedWidth(width) applied directly to content(), no inner Spacer wrapper. Five of the six centeredChatColumn callers are horizontally flexible and fill the column on their own; CompactionCircuitOpenBanner is the lone natural-width caller and gets an explicit HStack { Spacer; banner; Spacer } wrap at its call site to preserve centering. Resolved.

Comment on lines +549 to +554
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot May 2, 2026

Choose a reason for hiding this comment

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

🚩 Existing MessageListView fixedWidth usage pattern differs from new centeredChatColumn pattern

In MessageListView.swift:150, .fixedWidth(widths.chatColumnWidth) is applied directly to scrollViewContent without wrapper spacers, while the outer centering is handled by HStack { Spacer; content.fixedWidth(); Spacer }. This is the pattern used when content should fill the full column width (no centering within the fixed-width region needed). The new centeredChatColumn adds an additional inner HStack { Spacer; content; Spacer } layer before applying .fixedWidth(), creating a different layout structure. This inconsistency between the two patterns is worth noting — if all centeredChatColumn content should fill the full width (which appears to be the case for banners and the composer), the MessageListView pattern of direct .fixedWidth() application would be more appropriate.

Open in Devin Review

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

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot May 2, 2026

Choose a reason for hiding this comment

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

Agreed — the inconsistency is real. Follow-up #29232 brings centeredChatColumn in line with the MessageListView.swift:147-182 pattern (Spacer + content.fixedWidth(W) + Spacer, no inner wrapper). The one natural-width caller (CompactionCircuitOpenBanner) takes an explicit centering wrap at its call site rather than baking it into the helper. Resolved.

Spacer(minLength: 0)
}
}
Expand Down