perf(layout): override explicitAlignment to stop O(n×depth) cascade in custom Layout wrappers (LUM-1167)#28691
Merged
vex-assistant-bot[bot] merged 1 commit intoApr 28, 2026
Conversation
…n custom Layout wrappers (LUM-1167) BottomAlignedMinHeightLayout and WidthCapLayout were introduced to replace _FlexFrameLayout, but neither overrode Layout.explicitAlignment. The default protocol extension merges all subviews' alignment guides — the exact same O(n×depth) recursive cascade that _FlexFrameLayout performs. When BottomAlignedMinHeightLayout wraps the entire LazyVStack scroll content, this cascade walks every visible cell on each layout pass, producing 2000ms+ main-thread hangs (Sentry MACOS-M9, MACOS-JG). Override both explicitAlignment(of: HorizontalAlignment) and explicitAlignment(of: VerticalAlignment) to return nil on both layouts. Returning nil means 'no explicit guide — use default positioning', which is correct because both layouts position children via placeSubviews, not alignment guides. Closes LUM-1167 Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
Contributor
🤖 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:
|
Contributor
There was a problem hiding this comment.
Vex review ✦
Clean and correct. The explicitAlignment override returning nil is exactly the right fix — the default Layout protocol extension was silently re-introducing the same O(n×depth) cascade that BottomAlignedMinHeightLayout and WidthCapLayout were originally created to avoid.
A few notes:
- Both overrides (horizontal + vertical) are present — correct, since the default implementation merges both guide axes
placeSubviewshandles all positioning —nilhere has no behavioral side effect- The root cause analysis is well documented and accurately identifies the prior investigation miss (double-measure vs cascade)
- PR description correctly calls out AGENTS.md as needing an update for future custom
Layoutimplementations — should be followed up
Devin ✅ + Codex 👍 already in. CI passing. Merge criteria met.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overrides
Layout.explicitAlignmentonBottomAlignedMinHeightLayoutandWidthCapLayoutto returnnil, eliminating the O(n×depth) alignment-guide cascade that caused 2000ms+ main-thread hangs (Sentry MACOS-M9, MACOS-JG). Both layouts were introduced to replace_FlexFrameLayout, but neither overrodeexplicitAlignment— the default protocol extension merges all subviews' guides, which is the exact same recursive cascade_FlexFrameLayoutperforms. Returningnilis safe because both layouts position children viaplaceSubviews, not alignment guides.Root cause analysis
BottomAlignedMinHeightLayoutto replace.frame(minHeight:alignment:.bottom)and avoid_FlexFrameLayout. The implementation correctly usedplaceSubviewsfor positioning but missed theexplicitAlignmentoverride. PR perf: eliminate remaining FlexFrame anti-patterns from LazyVStack cell views (Batch 3) #26007/PR perf: cap ForEach item count and eliminate cell-level FlexFrames (LUM-945) #26092 createdWidthCapLayoutwith the same omission._FlexFrameLayoutspecifically, and (b) the defaultexplicitAlignmentprotocol extension. PR fix: replace FlexFrame with Layout protocol to eliminate main-thread hang (LUM-944) #26053 only addressed (a). The Apple docs state: "If you don't implement the method, the protocol's default implementation merges the subviews' guides."LayoutSubview.sizeThatFits()results internally, so the re-measure inplaceSubviewshits the cache and is cheap. The Sentry stack showingexplicitAlignmentin the call chain was the real signal.Layoutwrapper that exists to avoid_FlexFrameLayoutMUST overrideexplicitAlignmentto returnnil— otherwise the default extension re-introduces the same cascade. This should be documented in AGENTS.md for future Layout implementations.Prompt / plan
LUM-1167: Fix BottomAlignedMinHeightLayout double-measure hang. Deep investigation revealed the actual root cause is the missing
explicitAlignmentoverride, not the double-measure (which is SwiftUI-cached and cheap).Test plan
check-flexframe.sh: OK)explicitAlignmentreturningnilmeans "no explicit guide — use default positioning," identical to any view without custom alignment. Placement logic insizeThatFits/placeSubviewsis unchanged.