Fix layout-pass timing race in ScrollExpansion.updateHeight#5
Merged
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ExpandingViewController.viewDidLayoutSubviews kicks off updateHeight() on every outer hosting-controller layout pass. iOS 18 reordered those passes so the inner UIKit/SwiftUI layout (where the inner UIScrollView's contentSize gets assigned) can lag the first outer viewDidLayoutSubviews non-deterministically. When that happens, contentHeight is 0, diff = -visibleHeight, previousHeight is still nil — the existing guard takes the else branch and complete() fires while the snapshot is still exactly one screen-height tall. Defer completion when previousHeight == nil and contentHeight == 0: bump a retry counter, ask the host to schedule another layout pass via view.setNeedsLayout(), and return without completing. Cap the counter at 10 so a genuinely empty scroll view still terminates (and the existing 30s ExpandingViewController timeout remains the ultimate safety net). AppKitContainer gets the no-op default since this race is UIKit-specific. Related upstream work (unmerged): getsentry#183 (open) attempted to flush the inner layout synchronously via firstScrollView.layoutIfNeeded(). That flush isn't sufficient on iOS 18 when the SwiftUI host hasn't propagated the pass yet — the multi-pass retry approach handles that case.
dfed
force-pushed
the
dfed--default-core-animation
branch
from
721f88a to
78128f5
Compare
dfed
changed the title
- AppKitContainer overrides setNeedsAnotherLayoutPass to re-dispatch updateScrollViewHeight on the next runloop tick. The default no-op inherited from the protocol extension would hang the AppKit pipeline because updateViewConstraints is not re-triggered by needsLayout flags alone. - Refactor the contentSize-not-laid-out retry in updateHeight from an if/return into a guard with the negated condition, per the project's no-early-return-from-if rule.
Drop the runloop hop and the [weak self]. Setting needsLayout and synchronously re-entering matches the UIKit pattern; the retry counter in updateHeight bounds recursion at 10.
Previously the retry path returned as soon as contentHeight > 0, which let us commit at a partial height when SwiftUI hosting / UIKit child layout completes in multiple passes (a known iOS 18 race). Now we keep retrying until two consecutive passes report the same contentHeight — a stable observation — before proceeding. This eliminates a class of run-to-run dimension drift on iPhone 16 / iOS 18 captures where the inner scroll view advanced through several intermediate heights. Also adds previousHeight / pendingContentSizeRetries reset to AppKit's removeConstraints to match UIKit's hygiene.
…parent wrapper Two additional layout-pass timing flake classes that the existing contentHeight settle loop didn't catch: 1. Non-scroll intrinsic-content-size race UIHostingController's two-pass layout can drop intrinsic content size into the host view *after* the first viewDidLayoutSubviews — same race as the scroll-view path, but observed via systemLayoutSizeFitting / fittingSize instead of contentSize. Affected views: HomeFeatureLoreEntryViewController, HomeFeatureNotebookEntryViewController, etc. Add pendingIntrinsicSizeRetries / lastObservedIntrinsicSize + hostFittingSize on ScrollExpansionProviding. When firstScrollView is nil, wait for two consecutive identical fitting-size observations before completing — symmetric with the scroll-view path, capped at 10 retries. Falls through to immediate completion when the host has no useful intrinsic size (zero fitting size). 2. Safe-area inset resolution timing UIHostingController resolves safeAreaInsets across multiple layout passes (the host's insets propagate from the window / scene asynchronously), translating rendered content vertically run-to-run on layouts that read directionalLayoutMargins (e.g. HomeFeaturePhotoUploadViewController/Viewfinder showed ~150px vertical drift). Pin both insetsLayoutMarginsFromSafeArea = false and directionalLayoutMargins = .zero in ExpandingViewController init to eliminate that source of drift. AppKit gets the symmetric intrinsic-size additions (fittingSize as hostFittingSize, retry-state reset in removeConstraints). NSWindow's safeAreaInsets only apply to full-content layout windows and the BorderlessWindow used here doesn't trigger that, so no AppKit safe-area wrapper is needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Before this commit, the settle loop was driven from viewDidLayoutSubviews, which fires before viewDidAppear. SwiftUI @State / @published mutations set in viewDidAppear (a common pattern: viewModel.state = .someValue once the view is on screen) only propagate to the view tree after that, so the settle loop could observe a transient EmptyView / partial-content state, declare it stable, and lock in the wrong dimensions before the real content rendered. The canary was SelfRewardClaimViewController/Default/snapshot_a11y.png collapsing 1418 → 786 in some captures (a11y legend strip was absent because the SwiftUI body returned EmptyView while viewModel.state was still nil). FriendChatViewController/Default/snapshot_a11y.png showed the same family at smaller magnitude (24px height drift). Now updateScrollViewHeight is only called once viewDidAppear has fired. The retry-on-instability loop continues to run via setNeedsLayout → next viewDidLayoutSubviews, but only after the first viewDidAppear trigger. didAppear is reset in removeConstraints so the host VC can be reused for subsequent previews without leaking state. This is a UIKit-only change — AppKit's lifecycle and the failure mode (SwiftUI hosting controller layout pass reordering on iOS 18) are iOS-specific.
We don't exercise AppKit in our CI (iOS simulator only), so maintaining a synchronous-recursion implementation that has to coordinate AppKit layout pipeline correctness is out of scope. The protocol's no-op default lets the AppKit settle loop fall through to immediate completion. Cursor flagged a real bug (recursion exhausts retries without actual layout) but the right fix is 'don't try to handle AppKit at all' rather than maintain a correct AppKit implementation. The protocol-required stored properties stay so AppKit still compiles.
![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: 5194478503
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+130
to
+133
| func setNeedsAnotherLayoutPass() { | ||
| view.needsLayout = true | ||
| updateScrollViewHeight() | ||
| } |
There was a problem hiding this comment.
setNeedsAnotherLayoutPass() marks the view dirty and then immediately calls updateScrollViewHeight(), which re-enters updateHeight synchronously before AppKit has a chance to run another layout cycle. That means the new stabilization counters can burn through all retries in one call stack while reading the same stale size, then complete early with an unstable height. This is most visible when host/scroll sizes only converge after a later run-loop layout pass.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 79608ca. Configure here.
| // systemLayoutSizeFitting(compressedSize) and honors active layout | ||
| // constraints — same role in the settle loop. | ||
| view.fittingSize | ||
| } |
There was a problem hiding this comment.
AppKit hostFittingSize override prevents intended immediate completion
High Severity
AppKitContainer overrides hostFittingSize to return view.fittingSize (non-nil, typically non-zero), but its setNeedsAnotherLayoutPass() is a no-op (protocol default). The comment on lines 130–134 states the settle loop "falls through to immediate completion," which only works if hostFittingSize returns nil (the protocol extension default). With a real value returned, the non-scroll settle loop defers on its first observation (since lastObservedIntrinsicSize is nil ≠ fittingSize) and calls the no-op, so updateScrollViewHeight() may never be re-invoked — hanging the rendered callback indefinitely. The same issue affects the scroll path's new pendingContentSizeRetries guard.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 79608ca. Configure here.
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
1 participant
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.
Why
ScrollExpansion.updateHeighthas a layout-pass timing race that surfaces on iOS 18 / Apple Silicon hosting-controller layout-pass reorderings. Drift investigation on the apple-side consumer traced ~50 files of regen drift to this race and confirmed it persists on iOS 26 too — same layout pipeline, just lands "lucky" more often.The race surfaces three ways:
UIScrollView.contentSizelags the outerviewDidLayoutSubviews— captures land at one viewport height instead of the expanded content height.UIHostingController.intrinsicContentSizepropagates across multiple passes; captures sometimes bail before the second pass lands.HomeFeaturePhotoUploadViewControllershowed ~150 px run-to-run translation).Plus a related fourth case: SwiftUI
@Statemutations triggered inviewDidAppearpropagate after the firstviewDidLayoutSubviewswindow — settling on those passes locks in transient EmptyView / partial content (canary:SelfRewardClaimViewControllerwidth collapsed 1418 → 786 in some captures because the a11y legend depended on content the body wasn't yet rendering).iOS 26 hits the same races but lands "lucky" more often, so its regen counts are smaller. The bug is shared.
What changed
Stabilization-based settle loop in
ScrollExpansion.updateHeight:contentHeightbefore committing. NewpendingContentSizeRetries+lastObservedContentHeighton the protocol.view.systemLayoutSizeFitting(...)(UIKit) orview.fittingSize(AppKit). NewpendingIntrinsicSizeRetries+lastObservedIntrinsicSize+hostFittingSize. Returnsnil(immediate completion) for views that don't have a meaningful intrinsic size.ExpandingViewControllertimeout remains the ultimate safety net.Safe-area pin in
ExpandingViewController.init:So late-arriving safe-area insets can't translate captured content vertically. AppKit's safe-area model is window-level, so no AppKit-side change.
viewDidAppeargate for the UIKit settle loop.viewDidLayoutSubviewsno longer invokes the settle loop directly; instead,viewDidAppearflips adidAppearflag and kicks off the first pass. SwiftUI@Statepropagation triggered inviewDidAppearlands before we measure. Reset inremoveConstraintsso the host VC can be reused.AppKit: only the protocol-required stored properties are added on
AppKitContainer. NosetNeedsAnotherLayoutPassoverride — the protocol's no-op default takes over, and the AppKit settle loop falls through to immediate completion. We don't exercise AppKit in our CI (iOS simulator only) and don't want to maintain an AppKit-correct re-entry mechanism for code we don't actually run.Platform setNeedsAnotherLayoutPass on UIKit (
ExpandingViewController):view.setNeedsLayout()— relies on UIKit's natural pipeline to re-invokeviewDidLayoutSubviews. State is reset inremoveConstraints.Related upstream work
EmergeTools/SnapshotPreviews-iOS#183 (open since 2024-08-26, never merged) identified the inner
UIScrollView.contentSizerace but proposed only a single synchronousfirstScrollView.layoutIfNeeded()flush. That isn't sufficient on iOS 18 when the SwiftUI host hasn't propagated state yet — the multi-pass retry approach handles that case. This PR additionally addresses the non-scroll intrinsic-size race, the safe-area inset race, and theviewDidAppear-driven@Statepropagation race.