Repository navigation
fix(ios): show a page that SwiftUI hosts again after setPage - #1203
KisaneNeko wants to merge 2 commits into
Conversation
Each page is one React Native view, which PageChildViewController adds to its own view once, in viewDidLoad. SwiftUI can keep two controllers alive for the same page. The newer one takes the page view, and when SwiftUI removes it the view is left with no parent. If SwiftUI then shows the older controller again, nothing adds the view back, and the page is blank. When the collection view is about to display a cell, its controller now takes the page view back if the view has no parent. A view that a newer controller holds is left alone. Fixes callstack#1202
d15d62d to
e5a5b91
Compare
|
@KisaneNeko could you resolve the conflicts? |
…ter-setpage # Conflicts: # example/src/App.tsx
|
@apex-sentinel-staging review |
Apex SentinelVerdict: 1 finding(s): 1 nit. Verdict: approve. reached max rounds (1). 🔵 Nit (1)
Reply under an inline comment with |
There was a problem hiding this comment.
Apex Sentinel: 1 new finding on 83a74e1. The verdict and the full list are in the summary comment.
| @@ -84,6 +84,19 @@ class PagerScrollDelegate: NSObject, UIScrollViewDelegate, UICollectionViewDeleg | |||
|
|
|||
| func collectionView(_ collectionView: UICollectionView, willDisplay cell: UICollectionViewCell, forItemAt indexPath: IndexPath) { | |||
There was a problem hiding this comment.
🔵 Nit · willDisplay walks the cell's entire view tree recursively on every display
pageController(in:) is a full pre-order DFS over cell.contentView's subview tree, and collectionView(_:willDisplay:forItemAt:) fires for every cell that becomes visible — during every swipe, every reuse, and every time SwiftUI re-displays a cell after a re-render (exactly the re-render-on-every-event pattern the new repro screen exercises). The wrapped page view is an arbitrary React Native subtree: for a page containing a long ScrollView/list, any sibling branch that precedes the PageChildViewController.view branch is traversed in full before it is found, and each node runs an Objective-C next message plus a type check. Nothing memoizes the result, so the cost is O(subviews of the page) per display on the main thread during scroll animation, where a stutter is visible.
Suggested change
Avoid an unbounded recursive subview walk on the collection view's hot display path: bound the search depth (SwiftUI's hosting wrapper around the Representable's controller view is shallow) or cache the resolved PageChildViewController per cell so repeated willDisplay calls do not re-walk an arbitrary React Native subtree on the main thread.
Details
A cheaper version stops as soon as it reaches a UIHostingView boundary / the Representable's container instead of descending into arbitrary RN content, or only descends while no PageChildViewController has been found and bails once a subtree is known not to contain one (e.g. cache the resolved controller per cell).
Also raised here
Also raised at this location by maintainability: willDisplay reclaims the first PageChildViewController found in the whole cell subtree, not the cell's own — PagerScrollDelegate.swift:87-99 — pageController(in:) is a recursive depth-first walk that returns the first PageChildViewController it finds, so for a page that itself contains a nested PagerView (a supported pattern, and exercised by the existing Issue1156NestedHorizontalScrollRepro screen) the first hit can be an inner page's controller rather than the outer cell's. The reclaimWrappedViewIfOrphaned() call then acts on the wrong page — taking back an inner orphaned view — while the actually-orphaned outer page view stays parentless and blank, which is the bug this PR is fixing. The guard clause in reclaimWrappedViewIfOrphaned protects against stealing a live view, but it does not constrain which controller is picked. Searching the cell's own controller first (e.g. cell.contentView.next, then a depth-limited/subviews traversal that stops at any nested PageChildViewController it finds) would target the right page.
Also raised at this location by javascript: Reclaim lookup scans the page's whole view tree and can resolve to a nested pager's controller — The new pageController(in:) is an unbounded recursive DFS over cell.contentView and every descendant, returning the first PageChildViewController found in shallowest-first order, and it runs on every willDisplay (i.e. again on every cell reuse during a swipe). Two costs: (1) the traversal scales with the page's React Native view tree — hundreds of nodes for a real page, on the main thread, once per page appearance — while only one responder-chain answer is wanted; (2) view.next as? PageChildViewController is tested at every descendant, so a page that itself hosts a PagerView (a supported composition, e.g. example/src/gh-issues/Issue1156NestedHorizontalScrollRepro.tsx) can resolve to whichever controller's view sits shallower in the tree. When that is the inner controller, reclaimWrappedViewIfOrphaned() re-parents the inner pager's page view into the wrong controller as soon as its superview is momentarily nil, mutating a view tree the outer fix has no claim on. …
persona: swift-ios, javascript, maintainability, senior-developer
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
Summary
Fixes #1202. It surfaced while reviewing #1193, whose repro screen triggers it, but it happens on 9.0.10 without that PR.
On iOS, a page shown by
setPagecan be blank. The pager reports the page as selected, but the page has no content and stays empty after the pager stops.The recording in #1202 shows the bug.
What happened
PageChildViewControlleradds it to its own view once, inviewDidLoad.I traced this with logging: in the blank state, the visible cell's controller wraps the right page view, but that view has no superview.
Fix
The
collectionView(_:willDisplay:forItemAt:)handler inPagerScrollDelegatenow also finds the cell'sPageChildViewControllerbefore the cell shows. If the controller's page view has no parent, the controller takes it back.willDisplaya reused cell can still hold an older controller while SwiftUI is about to show a newer one. Taking the view unconditionally blanked page 0 in the basic example, which thebasic-pagere2e tests caught.viewDidLoadis unchanged.Test Plan
What's required for testing (prerequisites)?
iOS simulator or device. The new example screen, which re-renders the event log above the pager on every page, focus and keyboard event.
What are the steps to reproduce (after prerequisites)?
Added
Issue #1202 Blank Page Reproto the example app:Go 1.Go 2), page 2 (Go 1), page 1 (Go 0) and page 0 (Go 1).Go 2.Selected: 2, but the pager area is empty.Verified
This is a timing issue. Yesterday it reproduced every time: an e2e test for it failed 3/3 without the fix and passed 3/3 with it. Today the same test passes without the fix too, so I left it out of this PR.
The fix is based on the logs from the failing runs. When a page view has no parent, the fix puts it back. In every other case it does not change the view tree, and the pager behaves as before.
typecheck(root, example and e2e),lint(no warnings in the changed files),testand prettier pass.Android is untouched.
Compatibility
Checklist
README.md: not applicable🤖 Generated with Claude Code