fix(ios): improve TabView handling around more navigation controller#11225
Conversation
|
View your CI Pipeline Execution ↗ for commit 3b803c9
☁️ Nx Cloud last updated this comment at |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughiOS tab view delegate handling is refactored to expose the More navigation controller delegate as a public property wired through TabView initialization and disposal. After iOS clears this delegate on tab selection, the tab bar callback re-asserts it. Navigation callbacks now force layout updates to ensure late-attached pages render after push animations. ChangesiOS tab view delegate and layout handling
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/ui/tab-view/index.ios.ts (2)
137-144: 💤 Low valueConsider adding a defensive check for
moreNavigationControllerDelegate.The code re-asserts the delegate to ensure callbacks reach the TabView, but doesn't verify that
owner.moreNavigationControllerDelegateis non-null before assignment. While unlikely during an active tab selection callback, adding a check would align with the stated intent of "re-asserting" the delegate rather than potentially clearing it.🛡️ Proposed defensive check
// iOS lazily initializes moreNavigationController (e.g. on first access of // topViewController, or on first tap of the More tab) and can clear the // delegate we set in setViewControllers. Re-assert it whenever any tab is // selected so moreNav push/pop callbacks for tabs >= 5 still reach us. const moreNav = tabBarController.moreNavigationController; -if (moreNav && moreNav.delegate !== owner.moreNavigationControllerDelegate) { +if (moreNav && owner.moreNavigationControllerDelegate && moreNav.delegate !== owner.moreNavigationControllerDelegate) { moreNav.delegate = owner.moreNavigationControllerDelegate; }
606-606: ⚡ Quick winAdd null check on
moreNavigationControllerfor consistency.Line 142 guards access to
moreNavigationControllerwith a null check, but this line doesn't. While accessing properties on nil in Objective-C is safe (no-op), adding a consistent check improves clarity and defensive programming, especially sincemoreNavigationControlleris nil when there are fewer than 5 tabs.♻️ Proposed consistency fix
// When we set this._ios.viewControllers, someone is clearing the moreNavigationController.delegate, so we have to reassign it each time here. -this._ios.moreNavigationController.delegate = this.moreNavigationControllerDelegate; +if (this._ios.moreNavigationController) { + this._ios.moreNavigationController.delegate = this.moreNavigationControllerDelegate; +}
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 43fd6597-98a1-456b-98af-7fb25bcc3f08
📒 Files selected for processing (1)
packages/core/ui/tab-view/index.ios.ts
Co-authored-by: Nathan Walker <walkerrunpdx@gmail.com>
TabViewitems which could result in white screens on thePage/Framedue to timing around delegate events.