fix(macos): prevent Show less button from disappearing on user messages#26131
fix(macos): prevent Show less button from disappearing on user messages#26131devin-ai-integration[bot] wants to merge 3 commits into
Conversation
Decouple the user-message collapse decision from observed geometry so clicking Show less collapses the bubble instead of removing the button. .frame(height: 150) is a _FrameLayout, which hard-proposes 150pt to its child. onGeometryChange on the same subtree then reports the clamped height back into @State, flipping isCollapsible to false on the next render and tearing down both the button and the clamp together. The decision is now derived from a conservative estimate of text + per-attachment heights, which is deterministic from the model and not affected by layout. Worst case (estimate too low) gracefully degrades to no collapse button and natural-height rendering, matching existing non-collapsible user messages. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ SCROLL_STRATEGY.md documents removed userMessageIntrinsicHeight / onGeometryChange collapse logic (clients/macos/SCROLL_STRATEGY.md:211-217)
The PR removes userMessageIntrinsicHeight, onGeometryChange, and renames estimatedTextExceedsCollapseThreshold to estimatedContentExceedsCollapseThreshold, but clients/macos/SCROLL_STRATEGY.md:210-217 still documents the old two-phase approach with a code snippet referencing userMessageIntrinsicHeight and estimatedTextExceedsCollapseThreshold. Per the repo's AGENTS.md rule "When introducing, removing, or significantly modifying a service/module/data flow, update ARCHITECTURE.md and impacted domain docs," and the clients/AGENTS.md "Canonical reference" designation for SCROLL_STRATEGY.md, this documentation must be updated to reflect the new purely-deterministic estimation approach.
View 4 additional findings in Devin Review.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e733b1630
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let totalHeight = textHeight + imageHeight + videoHeight + audioHeight + fileHeight | ||
| return totalHeight > userMessageMaxCollapsedHeight |
There was a problem hiding this comment.
estimatedContentExceedsCollapseThreshold now decides collapsibility from textHeight + attachment heights only, but userMessageHeightWrapper clamps the entire bubbleChrome subtree, which always includes top/bottom padding (bubbleChrome adds VSpacing.md on both edges). This means messages with intrinsic content between ~126pt and 150pt are now classified as non-collapsible even though their rendered bubble exceeds the 150pt clamp threshold, so the "Show more" control disappears for a range of medium-length user messages that previously collapsed correctly.
Useful? React with 👍 / 👎.
…se estimate - SCROLL_STRATEGY.md: reflect the purely-deterministic content estimate (removes stale reference to userMessageIntrinsicHeight and estimatedTextExceedsCollapseThreshold, adds explicit no-geometry-in-state guidance with link to Apple docs). - ChatBubble.swift: include bubbleChrome vertical padding (2 * VSpacing.md) and inter-section VStack spacing (VSpacing.sm) in estimatedContentExceedsCollapseThreshold. Without this, messages with intrinsic content between ~126pt and 150pt render as bubbles >150pt but were classified as non-collapsible, hiding the Show more button. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
| + bubbleVerticalPadding | ||
| + interSectionSpacing | ||
| return totalHeight > userMessageMaxCollapsedHeight | ||
| } |
There was a problem hiding this comment.
Good catch — fixed in e9d78b7. The estimate now adds 2 * VSpacing.md (bubble chrome vertical padding) plus VSpacing.sm per inter-section gap in the inner VStack, so content between ~126pt and 150pt is now correctly classified as collapsible.
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
Product Context Flag
Before the code: LUM-833 is assigned to Tirman (Todo), and his PR #25579 takes a different direction — removing userMessageHeightWrapper entirely rather than fixing the collapse/expand behavior. Timur deferred to Tirman on this. If the product decision goes Tirman's direction (no collapsing at all), this PR becomes dead code.
This PR acknowledges the conflict in its description, but I want to flag it explicitly: the product decision on LUM-833 should land before merging this.
Code Review (if this approach proceeds)
The code itself is solid:
Correctness: The root cause analysis is accurate — .frame(height: 150) clamps the geometry observation, making isCollapsible flip to false on the next render. Removing onGeometryChange and driving from a deterministic content estimate breaks the feedback loop correctly. The Apple docs warning about geometry-driven state changes is cited properly.
Attachment height estimates (200pt single image, 120pt grid tile, 200pt video, 80pt audio, 40pt file) are conservative and the failure mode is graceful — worst case is a missing "Show more" button with content at natural height, same as non-collapsible messages today.
Anti-patterns: .frame(height:) stays (not .frame(maxHeight:)) — correct per our KB. No FlexFrame introduced. @State private var userMessageIntrinsicHeight is removed entirely, which is clean.
One question: The estimate uses NSFont.systemFont(ofSize: 14) for text height, but markdown headings render taller. Is there a real-world message where a heading-heavy message would be under 150pt estimated but over 150pt rendered, causing the Show more button to not appear when it should? Low risk since the failure is graceful, but worth a sanity check.
User messages render through MarkdownSegmentView using VFont.chat (16pt DM Sans). The estimate previously used NSFont.systemFont at 14pt, which under-counted text height and classified some messages that actually render > 150pt as non-collapsible. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
|
Good catch on the font — fixed in 9171e47. User messages render through For heading-heavy user messages specifically: headings do render taller than 16pt base, so in the rare case of a user message dominated by markdown headings (e.g., a 4-line message where each line is an On the LUM-833 conflict: agreed, flagging it for @Tirman's product call. |
|
Closing — deferring to Tirman's PR #25579 (LUM-833) which takes a broader approach to the user message collapse behavior. No point fixing the collapse/expand if it's being removed entirely. |
…UM-1108) Thread content visibly twitches/shifts left-to-right on initial load when switching to a thread containing a collapsible user message. The cause is a geometry-observation feedback loop in userMessageHeightWrapper: - .frame(height: 150) hard-proposes 150pt to its child (_FrameLayout). - onGeometryChange on the same subtree writes the clamped height back into @State userMessageIntrinsicHeight. - On thread switch, .id(conversationId) destroys the ScrollView, wiping @State. The first frame uses the NSString.boundingRect estimate as a fallback; subsequent frames re-observe the clamped height. The disagreement between the two drives the isCollapsible decision across two renders, flipping whether RoundedRectangle(.fill(surfaceLift)) background is applied. That background appearing/disappearing as container width resolves is the visible twitch. This is the SwiftUI anti-pattern Apple warns about in the onGeometryChange docs: geometry observations should not drive state that changes the observed layout. Fix: drive isCollapsible from a deterministic estimate of text + attachment heights, computed from the model. No geometry observation, no @State, no feedback loop. .frame(height:) stays (needed to avoid the _FlexFrameLayout hang from PR #24589). Worst case if the estimate under-shoots: no Show more button and content renders at natural height — identical to existing non-collapsible user messages. No clipping regression. Estimate covers text height (VFont.nsChat = 16pt DM Sans, matching MarkdownSegmentView), per-attachment heights (single image ~200pt, grid tiles 120pt, video ~200pt, audio ~80pt, file chip ~40pt), bubble chrome vertical padding (2 * VSpacing.md), and inter-section VStack spacing. Also fixes the Show less button disappearing on click (same root cause, originally diagnosed in closed PR #26131). Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…geometry (LUM-1108) Thread content visibly twitches/shifts left-to-right on initial load when switching to a thread containing a collapsible user message. The cause is a geometry-observation feedback loop in userMessageHeightWrapper: - .frame(height: 150) hard-proposes 150pt to its child (_FrameLayout). - onGeometryChange on the same subtree writes the clamped height back into @State userMessageIntrinsicHeight. - On thread switch, .id(conversationId) destroys the ScrollView, wiping @State. The first frame uses the NSString.boundingRect estimate as a fallback; subsequent frames re-observe the clamped height. The disagreement between the two drives the isCollapsible decision across two renders, flipping whether RoundedRectangle(.fill(surfaceLift)) background is applied. That background appearing/disappearing as container width resolves is the visible twitch. This is the SwiftUI anti-pattern Apple warns about in the onGeometryChange docs: geometry observations should not drive state that changes the observed layout. Fix: drive isCollapsible from a deterministic estimate of text + attachment heights, computed from the model. No geometry observation, no @State, no feedback loop. .frame(height:) stays (needed to avoid the _FlexFrameLayout hang from PR #24589). Worst case if the estimate under-shoots: no Show more button and content renders at natural height — identical to existing non-collapsible user messages. No clipping regression. Estimate covers text height (VFont.nsChat = 16pt DM Sans, matching MarkdownSegmentView), per-attachment heights (single image ~200pt, grid tiles 120pt, video ~200pt, audio ~80pt, file chip ~40pt), bubble chrome vertical padding (2 * VSpacing.md), and inter-section VStack spacing. Also fixes the Show less button disappearing on click (same root cause, originally diagnosed in closed PR #26131). Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Clicking Show less on an expanded user message currently hides the button without collapsing the bubble. This makes the expanded content feel stuck — there's no way to return to the preview state.
Root cause
userMessageHeightWrapperdecides whether a message is collapsible by writing the observed content height into@Statevia.onGeometryChange, and applying.frame(height: 150)to the same subtree when collapsed..frame(height:)is a_FrameLayoutthat hard-proposes 150pt to its child, so the observed height that gets written back into state is the clamped value. On the next renderisCollapsibleevaluates to150 > 150 == false, which removes both the frame clamp and the "Show less" button in the same pass — matching the reported symptom exactly.This is the SwiftUI anti-pattern Apple warns about in the
onGeometryChangedocs: geometry observations should not drive state that changes the observed layout. It was introduced by #24589 when.frame(maxHeight:)was swapped for.frame(height:)(a necessary perf fix —_FlexFrameLayoutinside LazyVStack cells caused 35s+ hangs).Fix
Drive
isCollapsiblefrom a deterministic estimate of text height plus per-attachment heights, computed from the model. No geometry observation, no feedback loop..frame(height:)stays (needed for perf).Worst case if the estimate under-shoots: no "Show more" button and content renders at natural height — identical to the existing behavior for non-collapsible user messages. No clipping regression.
What to review carefully
estimatedContentExceedsCollapseThreshold(200pt single image, 120pt per grid tile, 200pt video, 80pt audio, 40pt file). These mirror the renderers inChatBubbleAttachmentContent.swiftbut are deliberately conservative. Please sanity-check against the actual rendered sizes in the app.estimatedTextExceedsCollapseThreshold, but it's now the primary decision instead of a first-frame fallback.userMessageHeightWrapperentirely. This PR is narrower: it fixes the specific Show less bug without changing the 150pt preview product behavior. The two PRs will conflict; resolution should be driven by the LUM-833 product decision.Test plan
heuristicUserMessageCollapseWrapper, which is unchanged.Link to Devin session: https://app.devin.ai/sessions/c949e8ea53da4294962ad2cdc4874459
Requested by: @ashleeradka