fix(macos): drive user message collapse from the model, not observed geometry (LUM-1108)

[ashleeradka]

What changes

userMessageHeightWrapper in ChatBubble.swift no longer writes observed geometry into @State. The collapsibility decision is now derived from a conservative content estimate (text size + per-attachment heights) computed from the message model. .frame(height:) still clamps collapsed bubbles to 150pt.

A new rule in clients/macos/AGENTS.md codifies the underlying anti-pattern so it isn't reintroduced.

Why

The old code had a measurement feedback loop:

• .frame(height: 150) on the bubble subtree is a _FrameLayout — it hard-proposes 150pt to its child.
• An onGeometryChange inside that subtree observed the clamped height and wrote it to @State userMessageIntrinsicHeight.
• Next render, isCollapsible evaluates that state against 150pt and flips to false, removing the frame and the Show less button together.

This is exactly the anti-pattern Apple documents in onGeometryChange: geometry observations should not drive state that changes the observed layout. WWDC22's Compose custom layouts with SwiftUI and WWDC23: Demystify SwiftUI performance make the same point about layout/state coupling.

User-visible effects of the loop:

1. Horizontal twitch on thread switch (LUM-1108): .id(conversationId) destroys the ScrollView, wiping @State. First frame falls back to the NSString.boundingRect estimate; subsequent frames re-observe the clamped height. The disagreement flips isCollapsible, which gates the RoundedRectangle(.fill(surfaceLift)) background wrapper — that background appearing/disappearing as container width resolves is the twitch.
2. Show less button disappearing on click: toggling to collapsed re-applies the 150pt frame, the observer reports 150pt, isCollapsible becomes false, the whole wrapper (button + frame) tears down in one render.

Benefits

• Fixes LUM-1108 for Thursday's release.
• Incidentally fixes the Show less teardown bug (previously diagnosed in closed apps#26131).
• Removes one @State and one onGeometryChange from a hot chat path — less churn on every layout pass.
• Adds a durable guideline to AGENTS.md so the pattern is caught in review before it ships again.

Why it's safe

• No product behavior change. 150pt preview collapse still applies; Show more / Show less still work.
• Graceful degradation on underestimate. If the content estimate is low, the "Show more" button simply doesn't appear and content renders at natural height — identical to existing non-collapsible messages. No clipping regression.
• Perf-load-bearing .frame(height:) is preserved. .frame(maxHeight:) would regress the 35s+ LazyVStack hang fixed in apps#24589.
• Attachment heights are conservative. Sized to match the renderers in ChatBubbleAttachmentContent.swift; intentionally err toward overestimate so borderline content is classified collapsible rather than miscategorized.
• Heuristic fallback path unchanged. Messages past 3000 chars / 40 lines still use heuristicUserMessageCollapseWrapper.

Considered and rejected

• Narrow contentWidth == 0 guard in the estimator. My first instinct. Rejected because it addresses one symptom of the feedback loop, not the loop itself. Any future edit that re-introduces geometry observation would reopen the bug.
• Remove hideScrollIndicatorsBriefly() in MessageListScrollState. Initially hypothesized as the cause because it toggles .scrollIndicators on every thread switch. Rejected after the reporter confirmed the twitch is per-thread — only threads with a long user message reproduce. Indicator toggling can't explain content-dependent behavior. Left untouched here; it may still be worth removing as a separate architectural question (it's a sibling of the switchRestoreTask pattern the inverted-scroll migration explicitly retired), but mixing it into this PR would confuse the diff.
• Wait on apps#25579 / LUM-833 (which removes user-message collapse entirely). Rejected because [LUM-833] Fix long user messages getting cut off in chat view #25579 has been stalled for 8 days and the release is tomorrow. This PR preserves current product behavior; if LUM-833 later kills the feature, the wrapper gets ripped out wholesale — no conflict with this change.
• Apple's sizeThatFits on a custom Layout. Would give us measured size before the clamp, not after. Rejected as overkill: the deterministic model-driven estimate is simpler, doesn't add a new layout node, and the attachment counts give us the information we need without measuring.
• Deferring the collapse decision until after onGeometryChange fires once (without a clamp during that first pass). Rejected because it re-introduces the full-height flash the original NSString.boundingRect estimator was added to prevent (LUM-626, apps#24626).

Root cause analysis

How did the code get into this state?

The feedback loop was introduced in apps#24589 (Apr 8) when .frame(maxHeight:) was swapped for .frame(height:) as an emergency fix for a 35s+ LazyVStack hang caused by _FlexFrameLayout's O(n × depth) alignment cascade. The height-clamp changed from soft (maxHeight) to hard (height), but the onGeometryChange that had previously observed a soft-clamped subtree was kept unchanged. From that point on, the observed value was the clamped value — the loop was latent. Symptoms surfaced gradually: the Show less teardown bug was reported weeks later, and LUM-1108's twitch only reproduces when .id(conversationId) wipes the estimator's fallback, which requires thread switching on a thread with a collapsible message.

What decisions led to it?

• The perf fix (maxHeight → height) was correct and necessary, but the review didn't trace downstream consumers of the clamped subtree's geometry. Making a soft clamp hard changes what onGeometryChange reports.
• The two-phase estimator (NSString.boundingRect fallback + onGeometryChange primary) looked defensive but was actually masking the bug: most of the time the fallback was correct, so the loop's incorrect value was overwritten before the user noticed. The feedback loop only became visible when state-wipe (via .id()) made the fallback the rendered value.
• The three follow-up patches in Apr (apps#25309, the VFont.nsChat fix, the bubble-padding fix) all treated symptoms — miscategorized messages, wrong font, missing padding — without questioning whether the geometry observation itself should exist.

Warning signs we missed

• The onGeometryChange documentation directly warns against this pattern.
• The existing comment block at // MARK: - User Message Collapse / Expand (pre-change) described .frame(height:) as load-bearing for perf but didn't mention the measurement invariant it imposed on descendants.
• Closed apps#26131 diagnosed the exact feedback loop in writing 6 days before LUM-1108 was filed. It sat closed pending LUM-833, and the diagnosis didn't propagate into AGENTS.md — so LUM-1108 was triaged fresh without the prior analysis.

Prevention

This PR adds a rule to clients/macos/AGENTS.md:

Geometry observations must not drive state that changes the observed layout: if a subtree is size-constrained (e.g., .frame(height:), .clipped()) and an onGeometryChange or GeometryReader inside it writes the measured height/width into @State that gates the same constraint, you get a feedback loop — the observed value is the clamped value, so the decision to clamp flips off, the frame is removed, the child re-measures larger, the decision flips back on, and the layout oscillates or settles incorrectly. Derive layout-gating decisions from the model (content counts, text length, attachment types) or from a container-level geometry source that is not inside the constrained subtree. See onGeometryChange docs.

Secondary process suggestion (not blocking this PR): when a closed PR contains a root-cause analysis that wasn't acted on for non-technical reasons (product decisions, scope), the analysis should still be captured in the code review policy or AGENTS.md — closing a PR shouldn't close the learning.

Test plan

CI only covers lint + socket scans; macOS builds are skipped. Local Xcode verification:

• Switch between threads — twitch is gone on threads with a collapsible user message
• Long user message: Show more → expands; Show less → collapses to 150pt and button remains
• Short user message: no button, no clipping
• Single image + text pushing past 150pt: Show more appears and toggles cleanly
• Very long message (> 3000 chars / > 40 lines): heuristicUserMessageCollapseWrapper path triggers (unchanged)

🤖 Generated with Claude Code

[linear]

LUM-1108 Thread content twitches/shifts left-to-right on initial load (v0.6.5)

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[devin-ai-integration]

✅ Devin Review: No Issues Found

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

View in Devin Review to see 4 additional findings.

[vex-assistant-bot]

✦ APPROVE

Value: Fixes the geometry-observation feedback loop in user message collapse — the root cause of the thread-switch twitch (LUM-1108) and the "Show less" button teardown. Good for Thursday's release.

The feedback loop that was killed:

1. .frame(height: 150) clamps the child
2. onGeometryChange observes the clamped 150pt
3. isCollapsible evaluates 150 > 150 → false
4. Frame removed → child re-measures larger → isCollapsible flips back → oscillation

Classic anti-pattern Apple documents in the onGeometryChange docs.

What replaced it:
estimatedContentExceedsCollapseThreshold — purely model-driven, deterministic. Combines:

• NSString.boundingRect with VFont.nsChat (correctness fix over the old hardcoded systemFont(ofSize: 14) — now tracks actual rendered font)
• Conservative per-attachment heights mirroring ChatBubbleAttachmentContent.swift renderers
• Bubble chrome padding + inter-section spacing

Failure mode is safe: Underestimate → no "Show more" button → content renders at natural height. No clipping. Identical to a non-collapsible message.

Verified:

• @State userMessageIntrinsicHeight removed ✅
• onGeometryChange observer removed ✅
• Load-bearing .frame(height:) preserved (not .frame(maxHeight:) which causes the FlexFrame cascade) ✅
• partitionedAttachments already computed elsewhere in ChatBubble — no new work ✅
• stripHeavyContent / heuristicUserMessageCollapseWrapper fallback path untouched ✅
• AGENTS.md rule is well-worded and cites Apple docs ✅
• SCROLL_STRATEGY.md accurately updated ✅

Anti-patterns: None. This PR removes one (the geometry observation) and documents it as a rule to prevent reintroduction.

Clean fix, well-motivated, ships safely for Thursday.