fix(chat): eliminate _FlexFrameLayout alignment cascade in PinnedLatestTurnSection#29199
Conversation
… stop alignment cascade Replace .frame(minHeight:, alignment: .top) on PinnedLatestTurnSection with TopAlignedMinHeightLayout (Layout protocol). The flexible frame overload creates _FlexFrameLayout whose placeSubviews queries explicitAlignment on every descendant — O(n × depth) cascade causing 503+ Sentry hang/crash events (MACOS-63, MACOS-33, MACOS-50, MACOS-F, MACOS-RF). TopAlignedMinHeightLayout achieves identical sizing and placement via place(at:anchor:) and returns nil from explicitAlignment, stopping the cascade in O(1). Mirrors the proven BottomAlignedMinHeightLayout pattern. Also extends check-flexframe.sh to catch min(Width|Height) alongside max(Width|Height), and updates AGENTS.md to warn about ALL flexible frame parameters — the gap that allowed this regression. Fixes: LUM-1341 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:
|
![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: 73c4f5bf9d
ℹ️ 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".
| # Rust-regex compatible (no lookaround) so it works with ripgrep's default | ||
| # engine; we strip comment-only lines in a second pass below. | ||
| PATTERN='\.frame\(\s*max(Width|Height)\s*:' | ||
| PATTERN='\.frame\(\s*(max|min)(Width|Height)\s*:' |
There was a problem hiding this comment.
PATTERN only matches min/maxWidth and min/maxHeight, so new usages like .frame(idealWidth:) or .frame(idealHeight:) in the scanned chat/window paths bypass this guard even though they use the same flexible-frame overload (frame(minWidth:idealWidth:maxWidth:minHeight:idealHeight:maxHeight:)) that the updated docs now classify as unsafe. In practice this leaves a hole where a future _FlexFrameLayout regression can land without CI failing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — addressed in f62f147. The regex now catches ideal(Width|Height) alongside min/max variants, closing the gap for all six flexible frame parameters.
All six parameters on the flexible frame overload create _FlexFrameLayout. The regex now catches ideal* alongside min*/max* to close the remaining gap. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
| **Enforced mechanically in CI**: [`clients/scripts/check-flexframe.sh`](../scripts/check-flexframe.sh) fails the build on new `.frame(maxWidth:)` / `.frame(maxHeight:)` inside `Features/Chat/`, `Features/Home/`, and `Features/MainWindow/`. Known intentional leaves are listed in [`clients/scripts/flexframe-allowlist.txt`](../scripts/flexframe-allowlist.txt) with rationale. The manual audit process missed regressions twice (#25947 wrong call on bounded `maxWidth`, #26220 deferred leaves) before the lint existed — prefer fixing the code over adding allowlist entries; the allowlist is a last resort. | ||
| **Why Layout protocol wrappers are safe**: custom [`Layout`](https://developer.apple.com/documentation/swiftui/layout) implementations use [`LayoutSubview.place(at:anchor:proposal:)`](https://developer.apple.com/documentation/swiftui/layoutsubview/place(at:anchor:proposal:)) for positioning, which resolves the anchor from the child's known size as a `UnitPoint` — no alignment guide queries. The [default `explicitAlignment`](https://developer.apple.com/documentation/swiftui/layout/explicitalignment(of:in:proposal:subviews:cache:)-8cl0p) merges all subviews' guides recursively; overriding it to return `nil` tells ancestors "no explicit value; use default positioning", blocking the cascade in O(1). | ||
|
|
||
| **Enforced mechanically in CI**: [`clients/scripts/check-flexframe.sh`](../scripts/check-flexframe.sh) fails the build on new `.frame(minWidth:)` / `.frame(minHeight:)` / `.frame(maxWidth:)` / `.frame(maxHeight:)` inside `Features/Chat/`, `Features/Home/`, and `Features/MainWindow/`. Known intentional usages are listed in [`clients/scripts/flexframe-allowlist.txt`](../scripts/flexframe-allowlist.txt). Prefer fixing the code over adding allowlist entries; the allowlist is a last resort. |
There was a problem hiding this comment.
🟡 AGENTS.md CI enforcement description omits idealWidth/idealHeight despite script catching them
The CI enforcement description at line 318 lists only four flex-frame patterns (.frame(minWidth:) / .frame(minHeight:) / .frame(maxWidth:) / .frame(maxHeight:)) but the actual check-flexframe.sh regex (clients/scripts/check-flexframe.sh:72) is \.frame\(\s*(max|min|ideal)(Width|Height)\s*: which also catches .frame(idealWidth:) and .frame(idealHeight:). The rule description at clients/macos/AGENTS.md:304 correctly lists all six parameters, but the enforcement paragraph omits the two ideal* variants. An agent following the enforcement description would believe idealWidth/idealHeight are not CI-enforced and could be surprised by a lint failure.
Prompt for agents
In clients/macos/AGENTS.md line 318, the 'Enforced mechanically in CI' sentence lists .frame(minWidth:) / .frame(minHeight:) / .frame(maxWidth:) / .frame(maxHeight:) but omits .frame(idealWidth:) and .frame(idealHeight:). The CI script regex at clients/scripts/check-flexframe.sh:72 catches all six flex-frame parameters including ideal*. Add .frame(idealWidth:) and .frame(idealHeight:) to the enforcement description to match the actual CI behavior. The line is truncated in the diff but the full text should list all six patterns consistently with the rule description at line 304.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in be267bd — added idealWidth/idealHeight to the CI enforcement description to match the actual regex.
Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
…flexframe-cascade-lum-1341
devin-ai-integration
Bot
changed the title
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
✦ APPROVE
Value: Fixes an O(n × depth) explicitAlignment cascade in PinnedLatestTurnSection that's actively generating 503 Sentry events across 18+ users including one fatal EXC_BAD_ACCESS — replacing the one remaining .frame(minHeight:, alignment: .top) in the pinned turn section with the same Layout-protocol pattern that already eliminated the cascade for bottom-aligned content (PR #28691).
What this does: Introduces TopAlignedMinHeightLayout (mirrors BottomAlignedMinHeightLayout), wires it into PinnedLatestTurnSection via .topAlignedMinHeight(), extends the CI FlexFrame lint to cover all six flexible frame params (not just max*), and updates AGENTS.md + SCROLL_STRATEGY.md.
Implementation is correct — three critical things I verified independently:
-
Both
explicitAlignmentoverrides returnnil(horizontal and vertical). This is the only thing that actually stops the cascade. The defaultLayoutextension merges all subview guides recursively — missing either override would still trigger the O(n × depth) walk. ✅ -
placeSubviewsre-measures with its ownproposalparameter, not a stale one fromsizeThatFits. The comment says "same proposal thatsizeThatFitsreceived" — technicallyplaceSubviewsreceives its ownProposedViewSizeargument, and the code correctly uses that. Usingbounds.heightwould propose the expanded min-height back to the child and cause a size inconsistency SwiftUI detects and re-evaluates every frame. Code path is correct. (Minor comment wording nit, non-blocking.) ✅ -
Placement via
place(at: bounds.origin, anchor: .topLeading)correctly pins content to the top-left — equivalent toalignment: .topin the original.frame()call. ✅
CI lint extension is sound. All six flexible frame params (minWidth, minHeight, maxWidth, maxHeight, idealWidth, idealHeight) belong to the same overload that generates _FlexFrameLayout — the old max*-only pattern was always incomplete. The allowlist entries for ChatErrorToastView and ComposerView are legitimate: those minHeight uses are outside the LazyVStack ancestor chain. FlexFrame Lint CI is green. ✅
All bot-found issues addressed — Codex P2 (regex missing idealWidth/idealHeight), Devin --help truncation, Devin AGENTS.md description gap — all fixed and confirmed resolved by Devin's follow-up commits. ✅
Docs: AGENTS.md is now the most complete it's ever been on this topic — all six params documented, both the sizeThatFits (O(1)) vs placeSubviews (cascade via commonPlacement) distinction for .frame(width:) is explicit, and the "why Layout wrappers are safe" explanation is solid reference material for future Devin briefings.
One non-blocking observation: the no-subviews fallback in sizeThatFits (guard let child = subviews.first else { ... }) returns proposal.replacingUnspecifiedDimensions().width which is 10.0 for nil — but PinnedLatestTurnSection always has a child so this path is dead code in practice. Fine as-is.
1 participant
Replaces
.frame(minHeight:, alignment: .top)onPinnedLatestTurnSectionwithTopAlignedMinHeightLayout(Layout protocol) to eliminate the O(n × depth)explicitAlignmentcascade causing 503+ Sentry hang/crash events (18+ users, including a fatal EXC_BAD_ACCESS). Also extends the CI FlexFrame lint to catch all six flexible frame parameters and updates AGENTS.md accordingly.Why this change is needed
.frame(minHeight: viewportMinHeight, alignment: .top)atMessageListContentView.swift:498creates_FlexFrameLayout, whoseplaceSubviewsqueriesexplicitAlignmenton every descendant viacommonPlacement → ViewDimensions[guide]. InsidePinnedLatestTurnSection, this cascades O(n × depth) through the entire response cluster on every layout pass — triggered by viewport height changes during streaming, composer resizes, and.transition(.move)animations.Sentry evidence: 503 events across 5 issues (MACOS-63: 143, MACOS-33: 193, MACOS-50: 95, MACOS-F: 71, MACOS-RF: 1 fatal crash), active on v0.6.6.
Benefits
LayoutSubview.place(at:anchor:)minWidth,minHeight,maxWidth,maxHeight,idealWidth,idealHeight), not justmax*_FlexFrameLayout, rather than listing specific parameter names that create a false sense of completenessWhy this is safe
TopAlignedMinHeightLayout.sizeThatFitsreturnsmax(childHeight, minHeight)— same as.frame(minHeight:). The section still grows past the viewport for tall responses.bounds.originwith.topLeadinganchor — same visual result asalignment: .top.Layoutimplementations useplace(at:anchor:)which resolves anchor from the child's known size as aUnitPoint— no alignment guide cascade. ReturningnilfromexplicitAlignmenttells ancestors "no explicit value; use default positioning."BottomAlignedMinHeightLayout,FixedWidthLayout, andWidthCapLayout— all stable across multiple releases.Alternatives NOT taken
AlignmentBarrierLayoutwrapping each cell): Sound per Layout protocol, but unnecessary after removing the FlexFrame source. Previous attempts at barrier layouts between ScrollView and content broke scroll infrastructure — the barrier disrupted SwiftUI's internal scroll measurement. Cell-level barriers remain an option for defense-in-depth but are not needed for this fix..transition(.opacity)instead of.transition(.move()): Masks the cascade by avoiding uncached layout, but leaves the_FlexFrameLayoutin place. AGENTS.md correctly documents this as a workaround, not a fix.containerRelativeFrame(.vertical)directly on VStack: Caps section height at the viewport, making tall responses unreachable by scroll. This was the approach that created the need for.frame(minHeight:)in the first place — the current viewport-probe + minHeight pattern is load-bearing.Root cause analysis
How did the code get into this state?
containerRelativeFrame(.vertical)was introduced to fix one-frame anchor clipping during composer resizes. A review correctly identified it would cap tall responses at viewport height. The follow-up fix switched to.frame(minHeight:, alignment: .top)to allow growth — but this creates_FlexFrameLayout, the same internal type responsible for the original hang events.What mistakes or decisions led to it? The team's mental model associated "FlexFrame danger" exclusively with
maxWidth/maxHeight. AGENTS.md, CI lint, code reviews, and automated review bots all shared this blind spot. The flexible frame overload creates_FlexFrameLayoutfor ANY of its six parameters, but documentation only warned about two.Were there warning signs we missed? Yes —
MessageListView.swift:187-189already had a comment explaining why.frame(minHeight:alignment: .bottom)was replaced withBottomAlignedMinHeightLayoutfor this exact reason. The same pattern was introduced inPinnedLatestTurnSectionwithout connecting the dots.What can we do to prevent this pattern from recurring? The CI lint regex now catches all six flexible frame parameters. AGENTS.md has been rewritten to warn about the flexible frame overload generically rather than listing specific parameter names, and explains why Layout protocol wrappers are the safe alternative (with Apple doc links for independent verification).
AGENTS.md updates made in this PR: Rewrote the FlexFrame rule (line 304) to identify ALL parameters on the flexible frame overload as creating
_FlexFrameLayout. AddedTopAlignedMinHeightLayoutandBottomAlignedMinHeightLayoutto the safe alternatives list. Added an explanation of why Layout protocol wrappers are safe, citingLayoutSubview.place(at:anchor:)and the defaultexplicitAlignmentbehavior. Updated the CI enforcement description to list all six parameters.References
nilblocks the cascade_FlexFrameLayout_FrameLayoutPrompt / plan
See root cause analysis above. Investigated as LUM-1341.
Test plan