Skip to content

perf(chat): apply .fixedWidth() directly in centeredChatColumn (follow-up to #29231)#29232

Merged
vex-assistant-bot[bot] merged 1 commit into
mainfrom
devin/1777691205-centeredchatcolumn-direct-fixedwidth
May 2, 2026
Merged

perf(chat): apply .fixedWidth() directly in centeredChatColumn (follow-up to #29231)#29232
vex-assistant-bot[bot] merged 1 commit into
mainfrom
devin/1777691205-centeredchatcolumn-direct-fixedwidth

Conversation

@ashleeradka
Copy link
Copy Markdown
Contributor

@ashleeradka ashleeradka commented May 2, 2026

What

Drops the inner HStack { Spacer; content; Spacer } wrapper from centeredChatColumn, applying .fixedWidth(width) directly to content() — matching the established pattern in MessageListView.swift:147-182. Adds an explicit centering wrap at CompactionCircuitOpenBanner's call site, the only natural-width caller.

Why

Follow-up to #29231. The previous shape was:

HStack(spacing: 0) {
    Spacer(minLength: 0)
    HStack(spacing: 0) {                      // inner centering layer
        Spacer(minLength: 0)
        content()
        Spacer(minLength: 0)
    }
    .fixedWidth(width)
    Spacer(minLength: 0)
}

Inside the inner .fixedWidth(width) HStack, content with .frame(maxWidth: .infinity) or an internal Spacer is layout-flexible. Spacer is also flexible. SwiftUI's HStack distributes the proposed width among children sorted by flexibility; with two Spacer(minLength: 0) siblings sharing the proposal alongside flexible content, the result is order-dependent and not a documented invariant. The previous claim — "inner spacers collapse to zero and content fills the full column" — relies on undocumented HStack distribution behavior.

MessageListView.swift:147-182 already solved the same problem differently: apply .fixedWidth(W) directly to the content (scrollViewContent.fixedWidth(widths.chatColumnWidth)), and rely on FixedWidthLayout's contract — sizeThatFits returns exactly W, and placeSubviews proposes (W, height) to the single child. The child either fills W (flexible) or sits at top-leading (natural-width). No competition with sibling Spacers.

This PR adopts the MessageListView pattern in centeredChatColumn. Of the six callers, five are already horizontally flexible and fill the column on their own:

  • CreditsExhaustedBanner — inner VStack .frame(maxWidth: .infinity, alignment: .leading) (ChatErrorToastView.swift:223)
  • DiskPressureBannerSpacer(minLength: VSpacing.lg) between content and buttons (ChatErrorToastView.swift:263)
  • MissingApiKeyBannerVButton.frame(maxWidth: .infinity) (ChatErrorToastView.swift:405)
  • RecoveryModeBanner — inner VStack .frame(maxWidth: .infinity, alignment: .leading) (RecoveryModeBanner.swift:55)
  • ComposerSectionComposerView.frame(maxWidth: .infinity)
  • Read-only sentinel — HStack { Spacer; Icon; Text; Spacer } (already self-centers)

CompactionCircuitOpenBanner is the lone natural-width caller — a tight pill with icon + text and no horizontal flex (ChatErrorToastView.swift:339). The previous .frame(width:) semantics centered it implicitly; under FixedWidthLayout's topLeading placement it would render flush-left within the column. The fix wraps it explicitly at its call site so it stays centered — a more honest expression of the intent.

Benefits

  • Removes a layout subtlety. MessageListView and centeredChatColumn now use the same shape — Spacer + content.fixedWidth(W) + Spacer for page-level centering, with internal positioning handled at each call site as needed.
  • No reliance on Spacer-vs-flexible-content distribution. FixedWidthLayout proposes exactly width to its single child; the result is determined by the child's own size policy.
  • Helper docstring matches behavior. It centers the column on the page, not the content within the column.

Safety

  • Five flexible callers: behavior unchanged. Both old and new shapes propose width to content that fills horizontally.
  • CompactionCircuitOpenBanner: now wrapped at call site with HStack { Spacer; banner; Spacer }. Banner is non-flexible, Spacers split surplus, banner stays centered. Visually identical to pre-perf(chat): use FixedWidthLayout for centeredChatColumn (LUM-1276) #29231 behavior.
  • No additional _FrameLayout or _FlexFrameLayout introduced.

Alternatives considered

  • Make FixedWidthLayout accept a placement anchor parameter. Adds API surface to a primitive that intentionally exposes only width. Doesn't help: natural-width content centered inside a wider region would still need spacers somewhere. Rejected.
  • Add .frame(maxWidth: .infinity) to CompactionCircuitOpenBanner's outer HStack. Creates _FlexFrameLayout — the exact anti-pattern this layout family is replacing per clients/macos/AGENTS.md lines 304-309. Rejected.
  • Push centering into CompactionCircuitOpenBanner itself. Would change its visual from "tight pill at center" to "bar that spans the column with content leading-aligned." Different design intent. Rejected.

References

  • Layout.placeSubviews — single-child placement contract.
  • Layout.explicitAlignment — returning nil opts out of alignment-guide cascade.
  • clients/shared/DesignSystem/Modifiers/FixedWidthLayout.swift — reference implementation.
  • clients/macos/vellum-assistant/Features/Chat/MessageListView.swift:147-182 — established pattern.

Root cause analysis

  1. How did the code get into this state? perf(chat): use FixedWidthLayout for centeredChatColumn (LUM-1276) #29231 replaced .frame(width:) with .fixedWidth(...) inside centeredChatColumn, then added an inner HStack { Spacer; content; Spacer } to preserve the implicit .center alignment that .frame(width:) provided. The chosen shape works for fixed-size content but introduces an order-dependent behavior for flexible content.
  2. What mistakes or decisions led to it? Treating .frame(width:)'s alignment-as-default as needing direct reproduction inside the helper, instead of pushing it to the one caller that actually needed it. The existing MessageListView pattern (direct .fixedWidth()) was visible but not followed.
  3. Were there warning signs we missed? Yes — five of six callers were already self-filling, so the inner centering layer was load-bearing for exactly one banner. That asymmetry is a smell.
  4. What can we do to prevent this pattern from recurring? Use MessageListView's pattern as the canonical shape for chat-column wrappers. When introducing a Layout primitive, keep its contract narrow (one input, one output) and push positioning concerns to call sites.
  5. AGENTS.md update? No new rule needed. clients/macos/AGENTS.md lines 304-309 already prescribe .fixedWidth() over .frame(width:). The fix is to apply that rule consistently, not add more text.

Prompt / plan

Follow-up to PR #29231 addressing review feedback flagging the inner-Spacer competition with flexible content.

Test plan

  • Visual diff of all six centeredChatColumn call sites (5 flexible banners + composer + sentinel + natural-width CompactionCircuitOpenBanner).
  • Local Xcode build (no macOS CI runner).
  • Monitor MACOS-Q9 / MACOS-H7 fingerprints post-v0.7.1+.

Link to Devin session: https://app.devin.ai/sessions/9a108e31a87d4ebcb19aeefaff7f529a
Requested by: @ashleeradka


Open in Devin Review

…th banner explicitly

Apply .fixedWidth(width) directly to content in centeredChatColumn,
mirroring MessageListView's pattern. Wrap CompactionCircuitOpenBanner
(the only natural-width caller) in HStack { Spacer; banner; Spacer }
at its call site so it stays centered. The other five callers are
horizontally flexible and fill the column on their own.

Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
@devin-ai-integration
Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 1 additional finding.

Open in Devin Review

Copy link
Copy Markdown
Contributor

@vex-assistant-bot vex-assistant-bot Bot left a comment

Choose a reason for hiding this comment

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

APPROVE

Value: Tightens the fix from #29231 — eliminates the inner HStack { Spacer; content; Spacer } in favour of applying .fixedWidth(width) directly to content(), matching the MessageListView precedent. Cleaner and semantically unambiguous.


The argument against #29231's inner-Spacer pattern holds:
Inside a .fixedWidth(width) block, an HStack containing two Spacer(minLength: 0) siblings alongside flexible content has order-dependent distribution. The "inner spacers collapse to zero" claim is not a documented invariant. Valid concern.

New pattern is correct:
content().fixedWidth(width) passes exactly width as the proposal to the single child. FixedWidthLayout.sizeThatFits returns (width, childHeight). placeSubviews anchors at .topLeading — no guide query. Flexible content fills width; natural-width content sits at top-leading. Outer HStack { Spacer; ...; Spacer } handles horizontal centering on the page. Exact match to MessageListView.swift:147-182.

CompactionCircuitOpenBanner natural-width claim verified:
Read the source. HStack(spacing: VSpacing.sm) { VIconView; Text } — no Spacer, no .frame(maxWidth: .infinity). Genuinely natural-width; the explicit centering wrap at its call site is correct and necessary.

All checks green. Devin not yet reviewed — Vex + CI sufficient here given this is a one-function cleanup on a single-file diff authored by Boss.

@vex-assistant-bot vex-assistant-bot Bot merged commit ce28fb6 into main May 2, 2026
7 checks passed
@vex-assistant-bot vex-assistant-bot Bot deleted the devin/1777691205-centeredchatcolumn-direct-fixedwidth branch May 2, 2026 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant