fix(macos/chat): read chat widths from bubbleMaxWidth env, not static token (LUM-1117)#27557
Conversation
… (LUM-1117) Chat images, inline document previews, and thinking blocks all computed their width from the static VSpacing.chatBubbleMaxWidth (760pt) and fed it into a definite .frame(width:). At non-fullscreen window widths the content stayed 760pt wide and overflowed the actual chat column. ChatBubble already threads a container-aware bubbleMaxWidth via Environment, but these peer views never read it. Swap the static reference for an @Environment(\.bubbleMaxWidth) read in each site, falling back to the 760 default only when the env is 0 (first layout pass, per MessageListLayoutMetrics) or unset (non-chat contexts). Sites: - AnimatedImageView — markdown inline images. - InlineToolCallImageView — tool-generated images in the message flow. - AttachmentImageGrid — single-image attachments and pre-decode placeholder. Decode targetSize kept static so window resize doesn't re-fire the decode task on every frame. - InlineFilePreviewView — markdown budget for the expanded document card. - ThinkingBlockView — same pattern, inherited the same static cap. Shape stays .frame(width: computed) throughout; no .frame(maxWidth:) is introduced, so the _FlexFrameLayout hang patterns fixed in #25844, #26053, and #26092 stay out. Adds a durable AGENTS.md bullet describing the env-vs-token contract so the next editor doesn't reach for the static token by default. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
52debaf to
b78ccb7
Compare
There was a problem hiding this comment.
✦ APPROVE
Value: Fixes visible content overflow at non-fullscreen widths for images, inline file previews, and thinking blocks — all of which were stuck at static 760pt while the text path was already container-aware.
What this does: Migrates five chat-cell views from reading VSpacing.chatBubbleMaxWidth (static 760pt) to @Environment(\.bubbleMaxWidth) (container-aware value set by MessageListContentView). Adds an AGENTS.md rule documenting the env-vs-token contract so future contributors don't repeat the pattern.
Analysis — this is clean:
-
No anti-pattern violations. All
.frame()calls remain definite.frame(width:, height:)— no.frame(maxWidth:)introduced. FlexFrame Lint passes. The_FrameLayout→_FlexFrameLayoutboundary is preserved everywhere. -
Zero-width first-pass contract is correct. Every site follows the same
bubbleMaxWidth > 0 ? bubbleMaxWidth : VSpacing.chatBubbleMaxWidthguard pattern. WhenMessageListLayoutMetricsreports 0 beforeGeometryReaderresolves, the static fallback kicks in. This matches the existing text-path behavior, and the cache-poisoning guards from #26316 prevent degenerate measurements from persisting. -
Decode
targetSizecorrectly left static. The.task(id:)for ImageIO downsampling intentionally keepsVSpacing.chatBubbleMaxWidth— keying on a resize-varying env value would re-fire expensive downsampling every resize frame. The comment documents this trade explicitly. -
ThinkingBlockView simplification is a nice cleanup. Removed the paragraph-long comment explaining the old static math and replaced with a short, accurate comment about
maxContentWidthbecoming a definite.frame(width:). Less code, same behavior, now responsive. -
InlineFilePreviewView fix follows the same
- 2 * VSpacing.smpattern as ThinkingBlockView for the card padding subtraction. Consistent. -
Root cause analysis in the PR description is excellent. The "warning signs we missed" section identifying the pattern of one-off fixes (LUM-822, LUM-639, LUM-608) rather than sweeping is the real lesson here.
One observation (non-blocking): AnimatedImageView.gifSize(maxDimension:) is called twice in the GIF frame builder (once for width, once for height). It's trivial math so no perf concern — just noting if you ever wanted to hoist it into a local let.
Ship it.
What
Chat images, the inline document-preview card, and the thinking block all stayed at a fixed 760pt width at non-fullscreen window sizes and overflowed the chat column. This wires those five sites onto the existing container-aware
\.bubbleMaxWidthenvironment value thatChatBubble's main markdown path already uses. Adds a durable note toclients/macos/AGENTS.mddescribing the env-vs-token contract.Fixes LUM-1117.
Why it's safe
.frame(width: computed)throughout. No.frame(maxWidth:)is introduced — so the_FlexFrameLayouthang patterns fixed in #25844, #26053, #26092 don't return. See clients/macos/AGENTS.md § LazyVStack width/height rules and WWDC23: Demystify SwiftUI performance.MessageListLayoutMetricsdeliberately reportsbubbleMaxWidth = 0untilGeometryReaderresolves (see MessageListLayoutMetrics.swift:15-23); the cache-poisoning guards added in c7eefb41b5 already handle this for text, and images degrade to zero-size on the first pass (invisible, not oversized — same philosophy as the metrics comment).MessageListContentViewis the static 760 (ChatBubble.swift:11-13). Non-chat callers (e.g.SubagentDetailPanel) are unaffected.AttachmentImageGrid's decodetargetSizeremains static. Keying the decode.task(id:)on a resize-varying value would re-fire ImageIO downsampling every resize frame — the display frame uses the env, the decode does not. Trade: small memory overhead at narrow widths; avoided: resize-thrash.What I chose NOT to do
MarkdownSegmentView's default (fromVSpacing.chatBubbleMaxWidthto an env read).SubagentDetailPanelrendersMarkdownSegmentViewoutside the message list where\.bubbleMaxWidthis the static default; changing the internal fallback would silently reshape that panel's layout. Per-call-site env reads keep each context explicit..widthCap(bubbleMaxWidth).WidthCapLayoutis the right tool for content that proposes a natural width and needs to be bounded (WidthCapLayout.swift, blessed pattern per AGENTS.md:278). For these views the width feeds into a downstream computation (aspect-fit for images, definite.frame(width:)for measured text) — a cap modifier wouldn't reach those. Kept the existing.frame(width: computed)shape and swapped the input.ChatEmptyStateView/ChatLoadingSkeleton. Both reference the static cap but use.frame(maxWidth:)and live outsideLazyVStack, so they already shrink to container and don't hit the FlexFrame perf trap.SubagentDetailPanel. Separate surface, separate env context. ItsmaxContentWidth: nilis a different symptom of the same misleading API — worth a follow-up.Root cause analysis
How did the code get into this state?
.frame(maxWidth:, maxHeight:)with computed definite.frame(width:, height:)in image and file-preview paths to eliminate 2s+ FlexFrame hangs. The computation used the staticVSpacing.chatBubbleMaxWidth— correct for the perf problem at hand.MessageListLayoutMetrics+\.bubbleMaxWidthenv to make text content container-aware.ChatBubble's main markdown path was migrated to read the env. Peer views (images, document preview, thinking block) kept their static cap. There was no sweep.2 * VSpacing.smin the thinking block) patched symptoms within the static framework instead of migrating to the env.Decisions that led to it
\.bubbleMaxWidthexists but isn't discoverable from a peer view — you'd have to grepMessageListContentViewto find it.VSpacing.chatBubbleMaxWidth = 760reads like "the" chat bubble width. Nothing signals that it's a fallback and the env is the source of truth.MarkdownSegmentView.maxContentWidth. It reads like a max, butSelectableRunViewapplies it as a definite.frame(width:)— callers passing a static 760 ornil(which falls back to 760) are silently buggy at narrow widths. The comment at MarkdownSegmentView.swift:1041-1044 documentsnil = use defaultas intentional, which institutionalized the wrong pattern.Warning signs we missed
- 2 * VSpacing.smmath to compensate — a clear smell that the API was confusing.Prevention
MarkdownSegmentView.maxContentWidth→contentWidthor similar to signal "definite," or change the API to make it a real max.VSpacing.chatBubbleMaxWidthin chat-cell hierarchy to confirm no other latent overflow.@ViewBuilderhelper that wraps an env read + fallback so every site doesn't repeat the pattern.Prompt / plan
Enriched Linear ticket LUM-1117 (Garrison-reported cutoff at non-fullscreen widths) plus a user screenshot showing the document preview also overflowing. Investigation traced the static-760 pattern through every rich UI site in the chat feed, cross-checked the three FlexFrame-hang PRs to confirm the fix shape doesn't reintroduce them, and verified the first-layout 0-case handling before applying the env read at each site.
Test plan
./build.sh— green./build.sh lint— green (strict concurrency)MarkdownSegmentViewTests— 27/27, including the zero-width regression test from c7eefb41b5🤖 Generated with Claude Code