perf(macos/chat): drop redundant pinned-turn viewport probe

[ashleeradka]

Summary

Replace PinnedLatestTurnSection's local viewport-feedback loop with a chat-local EnvironmentValues channel sourced from the same viewportHeight MessageListView already filters via OnScrollGeometryChange.

• MessageListView declares a file-local ScrollViewportHeightKey: EnvironmentKey (mirroring BubbleMaxWidthKey in ChatBubble.swift) and publishes its filtered viewport height via .environment(\.scrollViewportHeight, …) on the ScrollView.
• PinnedLatestTurnSection reads @Environment(\.scrollViewportHeight), derives viewportMinHeight: CGFloat (defaulting to 0 before the first measurement lands), and applies it via the existing topAlignedMinHeight modifier. The containerRelativeFrame + onGeometryChange probe and its @State viewportMinHeight round-trip are removed.
• New AGENTS rule under "View Bodies and Rendering": "Don't double-track scroll viewport geometry."

Why this is needed

The pinned-turn section was sizing its topAlignedMinHeight floor against the scroll container with its own containerRelativeFrame + onGeometryChange probe, mirroring the resolved height into a local @State. MessageListView already tracks the same value via OnScrollGeometryChange to drive bottomAlignedMinHeight on the outer transcript stack — so the probe's @State round-trip was redundant and added another full layout pass through the eager MessageTranscriptStack VStack on every viewport change (composer multi-line resize, splitter drag, window resize, conversation switch). This surfaced as the LUM-1353 ~2s app hang.

Benefits

• Removes the redundant viewport-feedback loop and its extra layout pass per viewport change.
• Keeps the .equatable() barrier on MessageListContentView intact: viewport changes only re-run views that read \.scrollViewportHeight, not the whole transcript.
• Single source of truth for the visible viewport height — MessageListView's OnScrollGeometryChange — instead of two independent measurements that could drift.

Why this is safe

• The published value is the same filtered viewport height already feeding bottomAlignedMinHeight on the outer transcript stack — no new measurement, no new @State.
• The VSpacing.md * 2 subtraction matches the outer MessageListContentView.body padding (EdgeInsets(top: VSpacing.md, …, bottom: VSpacing.md, …)).
• viewportMinHeight defaults to 0 (not nil) before the first scroll-geometry measurement lands. topAlignedMinHeight(_:) is a @ViewBuilder modifier (if let minHeight { TopAlignedMinHeightLayout … } else { self }), so a nil → CGFloat transition would flip the section's structural identity and rebuild every transcript row inside on the first measurement. Defaulting to 0 keeps the TopAlignedMinHeightLayout wrapper present from frame 1, and minHeight: 0 has no sizing effect on non-negative content.
• EnvironmentKey and the EnvironmentValues extension are file-local (private struct + internal extension), matching BubbleMaxWidthKey — no design system surface area added.
• Internal-only SwiftUI layout change. No wire format, IPC, gateway endpoint, or NotificationCenter payload contract is touched, so it's compatible with both old-platform/new-macOS and new-platform/old-macOS skew.

Alternatives considered (and why not)

• Revert PinnedLatestTurnSection to the pre-pinned-turn rendering path. Rejected — the section exists to keep the user's latest message anchored to the visual top while a short response streams; reverting would regress the pinning UX.
• Keep the local probe but Equatable-shield the section's body. Rejected — @State mutation from .background is a SwiftUI anti-pattern (state-mutation triggered by layout) and would still measure the viewport twice. Removing the redundant probe is strictly better.
• Pass viewportHeight as an Equatable prop on MessageListContentView. Rejected — would either widen the == (re-evaluating the transcript on viewport changes) or require a separate non-equatable wrapper view. EnvironmentValues propagates orthogonally to == so the existing .equatable() barrier keeps doing its job.
• Promote ScrollViewportHeightKey to the design system. Rejected — only one consumer today; file-local matches the existing BubbleMaxWidthKey precedent. Promote later if a second consumer appears.
• Use Optional<CGFloat> and topAlignedMinHeight(nil) for the pre-measurement frame. Rejected — flagged by Devin Review: topAlignedMinHeight(_:) is a @ViewBuilder modifier with if let, so nil → value flips structural identity and rebuilds every transcript row on the first measurement. Defaulting to 0 keeps the wrapper stable.
• Add manual caching to BottomAlignedMinHeightLayout.placeSubviews. Out of scope. The hot path is the redundant feedback loop, not the layout itself; once the loop is removed the layout runs once per viewport change.
• Continue the observation-scope narrowing direction (PRs perf(chat): cache isPaginatedEmpty to decouple outer ChatView.body from message-array observation (LUM-1330) #29227 / perf(chat): extract ChatSearchOverlay into standalone View for narrow observation scope (LUM-1280) #29229). No clean next step inside MessageListContentView short of expanding into the deferred LazyVStack + MessageHeightCache rewrite. The viewport-feedback loop is a tractable, lower-risk fix landing this regression now.

Root cause analysis

1. How did the code get into this state? The probe was added to give PinnedLatestTurnSection a viewport-sized floor so the anchor row pins to the visual top while a short response streams. At the time the section was a localized leaf, so containerRelativeFrame inside .background looked self-contained.
2. What mistakes/decisions led to it? The viewport-feedback loop landed without checking whether MessageListView was already tracking the same value. By the time it shipped, OnScrollGeometryChange was already feeding bottomAlignedMinHeight from the same source, so the probe duplicated work and added another layout pass through the eager transcript stack on every viewport change.
3. Were there warning signs we missed? The probe wrote to @State from a .background view — state mutation triggered by layout, a known SwiftUI anti-pattern. Once the section grew to wrap the whole pinned-turn render, the @State round-trip was guaranteed to invalidate the section and re-measure the transcript on every viewport change. The Sentry signature on BottomAlignedMinHeightLayout.placeSubviews was the surfaced symptom.
4. What can we do to prevent this? Treat onGeometryChange → @State viewport feedback inside transcript-adjacent views as suspect. If a viewport-derived value already exists on an ancestor scroll container, propagate it through EnvironmentValues rather than re-measuring. Also: when swapping a non-optional @State for an optional environment value, check whether downstream @ViewBuilder modifiers branch on nil — default to the old initial value to keep view identity stable across the first read.
5. AGENTS.md update. Added a new bullet under clients/AGENTS.md "Performance and Resource Management → View Bodies and Rendering": "Don't double-track scroll viewport geometry." with links to onScrollGeometryChange, EnvironmentKey, and onGeometryChange. Concise, link-heavy, no PR number — follows the AGENTS.md philosophy.

References

• Apple — EnvironmentValues
• Apple — EnvironmentKey
• Apple — onScrollGeometryChange(for:of:_:)
• Apple — onGeometryChange(for:of:action:)
• Apple — ViewBuilder (if/else branches produce distinct structural identities)
• WWDC23 — Demystify SwiftUI performance
• Airbnb — Understanding and Improving SwiftUI Performance

Test plan

• clients/scripts/check-flexframe.sh passes with no new entries.
• Pre-commit secret/design-token guards pass.
• CI in this repo skips macOS/iOS builds, so the Swift compile and runtime behavior must be verified locally in Xcode.

Suggested local verification (Xcode):

1. Short response → anchor pins to visual top, spacer fills viewport.
2. Tall response (longer than viewport) → content remains scrollable, no clipping.
3. Composer multi-line Enter resize → no anchor row clipping or flicker.
4. Split-view / window drag-resize → no observable hitches.
5. Conversation switch with non-empty history → pinned-turn renders with viewport floor; no transcript-row rebuild flash on the initial frame.
6. Sentry: confirm MACOS-RW event count for BottomAlignedMinHeightLayout.placeSubviews drops post-deploy.

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

---

[linear]

LUM-1353

[devin-ai-integration]

🤖 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

[vex-assistant-bot]

✦ Vex Approval

Value: Closes the LUM-1353 ~2s app hang. Removes a redundant viewport probe in PinnedLatestTurnSection that was triggering a full eager-VStack layout pass through MessageTranscriptStack on every viewport change (composer multi-line resize, splitter drag, window resize, conversation switch). Routes the same filtered viewport height that MessageListView already publishes via OnScrollGeometryChange through a file-local EnvironmentKey, so descendants size against a single source of truth without taking their own measurement.

Boss's three explicit asks — all verified:

1. Environment injected outside the descendant tree, propagates to PinnedLatestTurnSection. ✅
   MessageListView.swift:230 applies .environment(\.scrollViewportHeight, viewportHeight.isFinite ? viewportHeight : nil) on the same chain as .scrollContentBackground / .scrollPosition / .scrollIndicators / .fixedWidth / .id(conversationId) / .flipped() — i.e. on the ScrollView itself. SwiftUI propagates env values to descendants, and PinnedLatestTurnSection (rendered inside via MessageListContentView → MessageTranscriptStack) reads it via @Environment(\.scrollViewportHeight).

2. VSpacing.md * 2 offset matches the old probe exactly. ✅

   • Old (MessageListContentView.swift:511-518 pre-PR): containerRelativeFrame(.vertical, alignment: .top) { length, _ in max(0, length - VSpacing.md * 2) } — closure subtracts VSpacing.md * 2 and clamps to non-negative.
   • New: private var viewportMinHeight: CGFloat { scrollViewportHeight.map { max(0, $0 - VSpacing.md * 2) } ?? 0 } — identical subtraction, identical clamp, default 0 instead of nil (deliberate; see #3).

3. EnvironmentKey is file-local in the same shape as BubbleMaxWidthKey. ✅
   Cross-checked against ChatBubble.swift:11-20 — private struct BubbleMaxWidthKey: EnvironmentKey { static let defaultValue: CGFloat = ... } + internal extension EnvironmentValues. New ScrollViewportHeightKey at MessageListView.swift:22-31 follows the exact same pattern: private struct, defaultValue CGFloat? = nil, internal extension property. The struct is file-private; the extension property is internal-by-default (required so MessageListContentView.swift in a sibling file can read it). Same scope envelope as the established pattern.

Devin's self-caught structural identity flip — verified and correctly resolved. ✅
The first push exposed viewportMinHeight: CGFloat? (nil before first measurement), which would have flipped PinnedLatestTurnSection's structural identity on the first measurement landing because topAlignedMinHeight(_:) is a @ViewBuilder with if let minHeight (verified at clients/shared/DesignSystem/Modifiers/TopAlignedMinHeightLayout.swift:73-83). The if let branch creates a TopAlignedMinHeightLayout wrapper; the else branch returns self. A nil → CGFloat transition appears the wrapper for the first time → SwiftUI rebuilds the entire pinned-section subtree (anchor row, response cluster, sentinel) on the first viewport measurement. Devin caught this and fixed it at 444807d by defaulting the computed property to 0 instead of nil. With 0, the wrapper is present from frame 1 and minHeight: 0 is a no-op against any non-negative content size — so the TopAlignedMinHeightLayout.sizeThatFits returns max(childSize.height, 0) = childSize.height and placeSubviews re-measures with the same proposal, no behavior change. The PR description also explicitly walks through this — good.

Cross-check vs the existing bottomAlignedMinHeight consumer:
Both env publish (line 230) and existing bottomAlignedMinHeight (line 191) use the identical filter viewportHeight.isFinite ? viewportHeight : nil. So the two consumers can never drift — when viewportHeight = .infinity (init or post-conversation-switch reset at line 233), both see nil simultaneously. Single source of truth confirmed.

Anti-pattern sweep — clean.

• No new .frame(width:/maxWidth:/maxHeight:) introduced in either file at HEAD cd70cb2f.
• The removed containerRelativeFrame + onGeometryChange + @State round-trip is itself an anti-pattern under the new "Don't double-track scroll viewport geometry" rule this PR adds.
• TopAlignedMinHeightLayout correctly opts out of the default explicit-alignment cascade (explicitAlignment returns nil) — verified at the modifier source.
• .equatable() barrier on MessageListContentView is preserved because the env channel only invalidates descendants that read \.scrollViewportHeight, not the parent — env propagation does not widen ==. Boss's PR description correctly identifies this as the reason env > prop-drill here.

Solid additions to the codebase:

• New AGENTS.md rule under "View Bodies and Rendering" — codifies the pattern so the next contributor doesn't rebuild a viewport probe inside a descendant of MessageListView. Cites onScrollGeometryChange / EnvironmentKey / onGeometryChange Apple docs. Excellent.
• The PR description's "Alternatives considered" section is exemplary — explains why simpler shapes (revert pinned-turn, Equatable-shield section, Equatable prop on MessageListContentView) don't work or trade off the wrong way. Future me will appreciate this.

Bot status:

• Devin Review found 1 potential issue (the structural identity flip) → resolved at 444807d with verification.
• Codex 👍 reaction on PR description ✅.
• Devin Review UI shows "2 additional findings" not visible via the GitHub API — recommend a quick scan via the Devin Review link before merge if anything looks worth addressing.

CI: Socket Security ×2 ✅, FlexFrame Lint ✅. macOS Build / macOS Tests / Lint Unused Code: SKIPPED — expected for clients/macos/ PRs that don't trigger the heavy macOS workflow. Boss QA on a local build is the gating factor before merge:

• cd ~/Development/vellum-assistant/clients/macos && ./build.sh clean && ./build.sh run
• Visual smoke test: open a chat with one short user message + short assistant reply (the section that uses PinnedLatestTurnSection), drag the window splitter, switch conversations, resize the composer multiline → confirm the latest turn stays anchored, no visible regression in pinning UX, no jump on first measurement.
• Sanity profile: trigger the same scenarios while running Instruments / spindump → confirm no BottomAlignedMinHeightLayout / FixedWidthLayout 2s hang.

Approving.

— Vex ✦