Repository navigation
Conversation
Fabric verifies child removals through the view manager's getChildAt, and TabsHostViewManager answered with the raw FrameLayout children, which never contain the tab screens. Every tab screen removal was therefore silently skipped, so remounted screens accumulated past the six-item bottom navigation cap and the next menu rebuild on re-attach crashed. Override getChildCount and getChildAt to answer from the rendered tab screens, matching ScreenStackViewManager and ScreenContainerViewManager.
…id-tab-screen-removals # Conflicts: # patches/react-native-screens/details.md
Fabric only asks for indexes it believes exist, but a null child makes it fall back to its usual already-removed handling instead of crashing with an out-of-bounds error. Also trims the comment on the overrides.
Native tab removals now go through, so the remount is only needed because the tab container cannot show a tab added after its first selection or drop the selected tab in place.
The merge's pre-commit hook rebuilt these bundles against outdated dependencies.
|
|
|
@mananjadhav Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
garrettmknight
left a comment
There was a problem hiding this comment.
Looks good from a product perspective.
mananjadhav
left a comment
There was a problem hiding this comment.
The code looks good. I'll work on the checklist.
|
@mountiny @garrettmknight Can we have an adhoc build here? Best to test on a physical device too. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-tab-count.mp4Android: mWeb Chromemweb-chrome-tab-count.moviOS: HybridAppios-tab-count.MP4iOS: mWeb Safarimweb-safari-tab-count.MP4MacOS: Chrome / Safariweb-tab-count.mov |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
mananjadhav
left a comment
There was a problem hiding this comment.
The lint failure seems to be unrelated to the PR.
…id-tab-screen-removals
…ts beta Main removed the Insights beta and its getTabWithoutBarItem helper, so the test now passes the tab without a bar item directly, and the comments no longer mention the beta. The comment and the patch details also say whose first selection leaves a later tab empty.
…id-tab-screen-removals
|
@mananjadhav need re-approval after merge latest main |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.5.28-4 🚀
|
|
No help site update needed. This PR fixes an Android crash in how the native tab bar removes tabs, and users see no change in tabs, labels, or workflows. |
|
The Deploy Blocker issue https://github.com/Expensify/Expensify/issues/699512 is repro on all PRs. Could you please double check if it's not introduced by #105020 |
|
I dont think this PR is relevant |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.5.30-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.5.30-0 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
On Android, the app could crash with
Maximum number of items supported by CustomBottomNavigationView is 6when the native tab bar rebuilt its items. The tab bar never removes a tab natively: React Native checks each removal against the parent's child views before carrying it out, and the tab bar keeps its tabs in fragments rather than as child views, so every removal is silently skipped. Each time a tab was swapped in place, the old one stayed in the bar next to its replacement, and the seventh one crashed the rebuild.This adds a react-native-screens patch that reports the tabs as the tab bar's children, the same way the library's stack already reports its screens, so removals go through. It is the same change as the open upstream fix, software-mansion/react-native-screens#5074 (software-mansion/react-native-screens#4812 rebased onto the current tabs layout).
Since #104595 removed the Insights beta, Account is always the tab without a bar item and nothing in the app adds or removes a tab at runtime, so current main no longer reaches this crash. The patch keeps the tab bar correct if a tab is added or removed at runtime again. The tab host remount in
NativeTabLayoutstays, because the native container still shows no content for a tab added after it selected its first tab, and still crashes when the selected tab is removed in place (upstream software-mansion/react-native-screens#4904 covers part of that). Its comment now says so, and a Jest test covers the remount.Upstream PR: software-mansion/react-native-screens#5074
Fixed Issues
$ #104505
PROPOSAL: #104505 (comment)
Tests
Since #104595, nothing in the app adds or removes a tab at runtime, so seeing the patch at work takes a dev build with two temporary edits.
useNativeTabNavigator.ts, let Debug mode pick the tab without a bar item. Replaceconst tabWithoutBarItem = NAVIGATORS.SETTINGS_SPLIT_NAVIGATOR;with:NativeTabLayout.tsx, removekey={tabsWithoutBarItemKey}from the view wrapping the tabs, so the tab host is updated in place instead of remounted.Onyx.merge('isDebugModeEnabled', true)in the React Native DevTools console, thenOnyx.merge('isDebugModeEnabled', false). Repeat both calls twice more.SettingsSplitNavigator, since main no longer gives it a bar label), and that nothing throws. On main, the bar shows six items after the first call (the removed tab is still there), and the second call throwsMaximum number of items supported by CustomBottomNavigationView is 6.Offline tests
Same as Tests. The change is in how the native tab bar removes tabs, which does not depend on the network.
QA Steps
Since #104595, nothing in the app adds or removes a tab at runtime, so QA can only check that the tabs still work.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Before (main, with the Tests edits):
104505-android-postmerge2-before.mp4
After (this PR, with the Tests edits):
104505-android-postmerge2-after.mp4
This PR, unmodified:
104505-android-postmerge2-regression.mp4
Android: mWeb Chrome
video-android-mweb.mp4
iOS: Native
video-ios-native.mp4
iOS: mWeb Safari
video-ios-safari.mp4
MacOS: Chrome / Safari
video-desktop-chrome.mp4