Skip to content

feat: add per-tab icon sizing - #608

Open
teddyxfire1992 wants to merge 4 commits into
callstack:mainfrom
teddyxfire1992:feat/tab-icon-size
Open

teddyxfire1992 wants to merge 4 commits into
callstack:mainfrom
teddyxfire1992:feat/tab-icon-size

Conversation

@teddyxfire1992

@teddyxfire1992 teddyxfire1992 commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

PR Description

Closes #561. Supersedes #562.

The feature was written by @thiagobrez in #562. This PR rebases it onto main and fixes two issues found while testing it on devices.

It adds a per-tab size for image icons:

  • Standalone: route.iconSize or getIconSize({ route })
  • React Navigation / Expo Router: tabBarIconSize

If the size is omitted, zero or invalid, the platform default is used (27 pt on iOS, Material's default on Android). Sizes go to native as a parallel iconSizes codegen array.

Changes on top of #562:

  • iOS, rebase onto main: TabViewImpl now uses the tab bar item image cache from main, so iconSize is added to the cache inputs and passed to makeTabBarItemImage.
  • iOS, SwiftPM build: ios/Bridge/BottomTabsBridge.h/.m did not forward the new prop, so the SwiftPM example did not compile (property 'iconSizes' not found on object of type 'TabViewProvider *' and no known class method for selector 'createWithKey:...iconSize:...'). The bridge now declares iconSizes and passes iconSize to TabInfo. The CocoaPods build was not affected.
  • Android, size reset after load: icons load asynchronously, and the load callback used the TabInfo captured when the request started. If the items were updated while the icon was loading, the callback applied the old size, so a 34 dp icon could end up at 24 dp. The callback now reads the current item, and it skips a result whose size is out of date, because a request for the new size is already running.
  • The artifacts/issue-561/ folder (screenshots, videos and hierarchy dumps, about 3.6 MB) from feat: add per-tab icon sizing #562 is not included.
  • Example: a new Icon sizes example page, based on Four Tabs, shows the default size next to sizes 20, 32 and 40. The other examples are unchanged.

How to test?

  1. Run the example app and open Icon sizes.
  2. Check that the tabs show four different icon sizes: Default, 20, 32 and 40.
  3. Switch between the tabs a few times and reopen the screen. The sizes must stay the same, also on Android.
  4. Build the SwiftPM example (apps/example-swiftpm). It must compile.

Verified with agent-device:

  • iPhone 17 Pro simulator, iOS 26.5: the four tabs show four different sizes, and selection works.
  • Pixel 10 Pro XL emulator (3x density): the icons measure 72, 60, 96 and 120 px, which is 24 dp (the default), 20, 32 and 40 dp. Selection works.
  • yarn typecheck and yarn lint (3 existing warnings, no errors) pass.

Known limitations, not changed here:

  • With experimental_bakedTintColors on iOS, the label of a larger icon is about 4 pt lower than the other labels.
  • On Android, the label moves down as the icon gets larger, so the labels of different sizes do not line up.

Screenshots

The Icon sizes example.

iOS Android
iOS: tab icons at the default size and at 20, 32 and 40 pt Android: tab icons at the default size and at 20, 32 and 40 dp

Recordings

Switching between all tabs of the Icon sizes example.

iOS

icon-sizes-ios.mp4

Android

icon-sizes-android.mp4

Comment thread packages/example-shared/src/Examples/NativeBottomTabsOriginalIcons.tsx Outdated
Comment thread packages/react-native-bottom-tabs/src/__tests__/index.test.tsx Outdated
Comment thread packages/react-navigation/src/__tests__/NativeBottomTabView.test.tsx Outdated
Comment thread packages/react-navigation/package.json Outdated
Comment thread packages/react-navigation/package.json Outdated
Comment thread packages/react-native-bottom-tabs/package.json Outdated
Comment thread packages/react-native-bottom-tabs/package.json Outdated
Adds `iconSize` / `getIconSize` for standalone usage and `tabBarIconSize`
for React Navigation. Omitted, zero or invalid sizes keep the platform
default.

Originally opened as callstack#562, rebased onto main and adapted to the tab bar
item image cache. The SwiftPM bridge now forwards `iconSizes` and
`iconSize` so the SwiftPM build compiles.
Icon load callbacks applied the tab item captured when the request
started. When `iconSizes` arrived while an icon was still loading, the
late callback reset the icon to the default size. Read the latest item
when the icon is ready and drop results loaded at an outdated size.
Adds an "Icon sizes" example based on Four Tabs that shows the default
size next to 20, 32 and 40, and removes `tabBarIconSize` from the
original icons example.
Restores the placeholder test in `react-native-bottom-tabs`, removes the
React Navigation test and rolls back the added jest and
react-test-renderer dev dependencies.
@thiagobrez

Copy link
Copy Markdown
Member

@apex-sentinel-staging review

@apex-sentinel-staging

Copy link
Copy Markdown

Sentinel reviews this repository only for the account that connected the installation, and for logins in user_settings.reviewers on the default branch.

@thiagobrez

Copy link
Copy Markdown
Member

/review

@thiagobrez

Copy link
Copy Markdown
Member

@apex-sentinel-staging review

@apex-sentinel-staging

Copy link
Copy Markdown

Apex Sentinel

Verdict: request_changes

20 finding(s): 3 major, 11 minor, 6 nit. Verdict: request_changes. reached max rounds (1).

Required fixes

  • iOS: per-tab iconSize is dead in the default (non baked-tint) path — SFSymbol tabs and the SwiftUI label icon ignore it
  • Android: icon scale is applied to only part of the drawable cache after an iconSize change
  • Android: setIconSizes(null) wipes every per-tab iconSize, including route.iconSize

🟠 Major (3)

Where Finding
packages/react-native-bottom-tabs/ios/TabViewImpl.swift:315 iOS: per-tab iconSize is dead in the default (non baked-tint) path — SFSymbol tabs and the SwiftUI label icon ignore it
packages/react-native-bottom-tabs/android/src/main/java/com/rcttabview/RCTTabView.kt:431 Android: icon scale is applied to only part of the drawable cache after an iconSize change
packages/react-native-bottom-tabs/android/src/main/java/com/rcttabview/RCTTabViewManager.kt:131 Android: setIconSizes(null) wipes every per-tab iconSize, including route.iconSize

🟡 Minor (11)

Where Finding
packages/react-navigation/src/views/NativeBottomTabView.tsx:28 NativeBottomTabView rebuilds navigationState and every route object on each render, defeating TabView's memos and re-rendering all scenes
packages/react-native-bottom-tabs/android/src/main/java/com/rcttabview/RCTTabView.kt:47 Android: duplicate drawables per icon size, and every size variant is kept for the view's lifetime
packages/react-native-bottom-tabs/android/src/main/java/com/rcttabview/RCTTabView.kt:444 Android: per-tab size relies on Material's restricted NavigationBarItemView class
packages/react-native-bottom-tabs/android/src/main/java/com/rcttabview/RCTTabView.kt:444 Android: updateIconSize()/icon layout is never reset when a tab has no iconSize (and after a configuration change)
packages/react-native-bottom-tabs/ios/TabViewProvider.swift:288 iOS: an icon load can be stamped with the size of a different route after the route list shrinks, leaving a stale icon until the next reload
packages/react-native-bottom-tabs/android/src/main/java/com/rcttabview/RCTTabView.kt:65 Android drawable cache is keyed per size and never evicted
packages/react-native-bottom-tabs/android/src/main/java/com/rcttabview/RCTTabView.kt:311 Android: setIconSizes runs a full updateItems pass on every prop update, re-enqueuing every icon load and every badge
packages/react-native-bottom-tabs/android/src/main/java/com/rcttabview/RCTTabView.kt:360 Android: setIcons resolves a missing item to the default icon size instead of deferring, so a default-size drawable can be applied before the real size is known
packages/react-native-bottom-tabs/android/src/main/java/com/rcttabview/RCTTabView.kt:444 updateIconSize silently no-ops while the tab item view is not laid out yet, and nothing retries
packages/react-native-bottom-tabs/ios/Bridge/BottomTabsBridge.h:50 RNCTabInfo createWithKey:...: gains a required iconSize: argument — source-breaking for SwiftPM consumers, undocumented
packages/react-native-bottom-tabs/ios/TabViewImpl.swift:407 iconSize in the tab-bar item image cache is dead on the default iOS path, so a size change re-renders every item image
🔵 Nit (6)
Where Finding
packages/react-native-bottom-tabs/ios/TabViewImpl.swift:532 The 27 pt iOS default is duplicated as a literal instead of using defaultIconSize
packages/react-native-bottom-tabs/android/src/main/java/com/rcttabview/RCTTabView.kt:248 val items = items.mapIndexed { ... } shadows the parameter and the items property
packages/react-native-bottom-tabs/ios/RCTTabViewComponentView.mm:126 iOS: itemsData is assigned before iconSizes, so updateItems() runs once with the stale sizes array
packages/react-native-bottom-tabs/ios/RCTTabViewComponentView.mm:234 iconSize is always passed as a constant from the prop conversion on both platforms
packages/react-native-bottom-tabs/ios/TabViewProvider.swift:337 updateItems() reloads icons on a size diff only, so a size change arriving with a new icon source in one commit can resize the old image
packages/react-native-bottom-tabs/ios/TabViewProvider.swift:353 iOS per-tab sizing depends on UIImage alignment rects being honored by UITabBarItem

Reply under an inline comment with @apex-sentinel-staging note ... or @apex-sentinel-staging pivot ... to re-check that finding.
Comment @apex-sentinel-staging review on this conversation to run a full review.

@apex-sentinel-staging apex-sentinel-staging Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apex Sentinel: 20 new findings on 398b6bc. The verdict and the full list are in the summary comment.

icon: assetIcon.map(IconSource.image) ?? .sfSymbol(tabData.sfSymbol),
focusedIcon: focusedAssetIcon.map(IconSource.image) ?? tabData.focusedSfSymbol.map(IconSource.sfSymbol),
title: tabData.title,
iconSize: iconSize(for: tabData),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Major · iOS: per-tab iconSize is dead in the default (non baked-tint) path — SFSymbol tabs and the SwiftUI label icon ignore it

TabViewImpl.iconSize(for:) is only consumed inside TabBarItemImageCache.Inputs, i.e. it only changes the rendering done by makeTabBarItemImage, which is used when inputs.rendersLabelIntoImage is true — that requires useBakedTintColors (experimental_bakedTintColors + iOS 26) and hasCustomTintColors && labeled. In every other configuration the images that actually reach the tab bar are produced by renderTabBarIcon(...), which just returns icon / icon.withTintColor(...) at the icon's own size, and the unselected icon is owned by SwiftUI through the tab label. The only code that applies iconSize for asset icons is TabViewProvider.loadIcons (image.resizeImageTo(size: iconSize(at: index))), so:

Suggested change

Apply iconSize on the path that is always taken (resize the image used for the tab label / selected image, including SF Symbols via a symbol configuration with pointSize = iconSize), or document that iconSize only affects asset icons and only on the rendering path where it is actually applied.

Details
  • An SF Symbol tab (route.focusedIcon: { sfSymbol: '...' }, no asset) never gets resized: loadIcons only iterates the icons array built from RCTImageSources (asset/uri icons), so props.icons[itemIndex] is nil and makeSFSymbolImage(named:) produces a symbol rendered at UIKit's default size. Passing tabBarIconSize: 40 on such a tab changes nothing on iOS unless experimental_bakedTintColors is on.
  • Same for the unselected icon in general: it comes from the SwiftUI label, not from makeTabBarItemImage, so a per-tab size is not applied to it outside the baked-tint path.

The PR's device verification used PNG assets in the example, which is why the feature appears to work there. Either resize the icon in renderTabBarIcon/the label image (and for SF Symbols via UIImage.SymbolConfiguration(pointSize:)), or state that iconSize applies to asset icons only in the docs added in this PR.

Also raised here

Also raised at this location by jvm: SwiftUI path never receives iconSize: TabItem builds Image without it, so SF Symbols and tab-bar background images ignore the new size — Everywhere else the size never reaches the rendered icon: TabItem.swift (unchanged by this PR) builds Image(uiImage:) without passing the new iconSize parameter, and makeTabBarItemImage is the only place that draws an icon at a requested point size. So on the default iOS path only image icons resized in TabViewProvider.loadIcons honour iconSize; SF Symbol tabs (UIImage(systemName:), sized by SwiftUI's font) and the non-baked label/selected images stay at the system size, while the doc page added in this PR states the option is the "width and height of the image icon" for any tab. The changeset and docs promise the option unconditionally, so either the size has to be applied on the SwiftUI/UITabBarItem path too (e.g. symbol configuration / image resizing for SF Symbols) or the docs and the option comment must say it only affects loaded image assets (and, for the baked-label composite, only in combination with baked tint colors).

Also raised at this location by cpp: iOS: iconSize / tabBarIconSize has no effect on SF Symbol tabs outside the experimental baked-tint path — Correction/精确 scope of my earlier finding (which overstated the case): asset icons are sized on iOS in the default path, because TabViewProvider.loadIcons resizes before storing into props.icons/props.focusedIcons, and NewTabView/LegacyTabView render props.icons[index] while configureTabBarItemImages assigns the focusedIcons-derived image. The gap that remains is SF Symbols: For a tab whose icon is an Apple Symbol (focusedIcon: { sfSymbol: 'house' }, or tabBarIcon: () => ({ sfSymbol: 'house' }) in the React Navigation wrapper), no image ever passes through loadIcons — resolvedIconAssets puts { uri: '' } for those routes — and the icon is produced by makeSFSymbolImage(named:) / Image(systemName:) in TabItem. Neither is configured with the requested point size, and renderTabBarIcon returns the symbol untouched. …

persona: cpp, jvm

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.

).roundToInt()
}

private fun applyIcon(menuItem: MenuItem, index: Int, iconSizePx: Int, drawable: Drawable?) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Major · Android: icon scale is applied to only part of the drawable cache after an iconSize change

updateItems recomputes sizes but never clears drawableCache, and getDrawable returns a cached drawable before its size check (drawableCache[cacheKey]?.let { onDrawableReady(it); return }). After an iconSize change, applyIcon compares getIconSize(item) with the requested iconSizePx and silently returns when they differ, trusting that "a request for the new size is already running". That only holds for entries that were not in the cache yet. The entries that were already cached under the old size are never re-requested for the new key (the loop always requests the new key, which misses and starts a load; the old key's drawable stays in the map), so the tab keeps whatever menuItem.icon had before, and the guard drops the callback of any in-flight load for the old size. Failing sequence: an iconSize/tabBarIconSize update (or a route reorder that shifts iconSizes positionally, since updateItems maps sizes by index, not by key) while an icon for the old size is still loading → the item is left at the old size with no further request. Also note the post { } inside Coil's target (line ~406) already delays the callback to the end of the queue, so an old-size load finishing after the new-size load can win the last write to menuItem.icon and then be discarded by the guard, leaving the old drawable assigned with no retry. Clearing the cache in updateItems/setIconSizes (or applying the item's current size to the drawable instead of dropping the mismatch) removes the ordering dependency.

Suggested change

In updateItems(), drop the cache entries for the icon sources whose resolved size changed (or clear drawableCache wholesale) so every visible item has a request for its current size, instead of relying on the "a request is already in flight" assumption in applyIcon.

Details

persona: jvm

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.

view.setIcons(value)
}

override fun setIconSizes(view: ReactBottomNavigationView?, value: ReadableArray?) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Major · Android: setIconSizes(null) wipes every per-tab iconSize, including route.iconSize

RCTTabViewManager.setIconSizes maps a missing/null prop to emptyList(), and ReactBottomNavigationView.setIconSizes stores it and re-runs updateItems(items), whose first statement recomputes every item as item.copy(iconSize = iconSizes.getOrNull(index)?.takeIf { it > 0 }). With the parallel array empty, getOrNull(index) is null for every index, so every item's iconSize becomes null and getIconSize falls back to bottomNavigation.itemIconSize.

Suggested change

Treat a null iconSizes array as "no sizes provided" and skip the update (or keep the previous array), so that clearing the prop does not silently drop sizes; if clearing is intended, note it in the changeset.

Details

iconSizes is an optional prop of the native component, so this is reachable: any consumer that passes getIconSize/tabBarIconSize on one render and then renders without the prop (or an out-of-tree/dynamic caller that omits it) gets all icons reset to the default size, whereas before this PR there was no size at all and the default was the only behavior. Because items is overwritten with the recomputed copies, the original per-route sizes cannot be restored either — the information is lost, and a later updateItems(items) (e.g. from onConfigurationChanged) keeps the defaults. A null/absent iconSizes should leave this.iconSizes and the existing sizes untouched (or be treated as "no change"), not as "clear all sizes".

persona: cpp

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.

@@ -28,7 +28,13 @@ export default function NativeBottomTabView({
return (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor · NativeBottomTabView rebuilds navigationState and every route object on each render, defeating TabView's memos and re-rendering all scenes

navigationState={{ ...state, routes: state.routes.map(route => ({ ...route, iconSize: ... })) }} creates a new state object, a new routes array and new route objects on every render of the navigator. TabView keys trimmedRoutes on navigationState.routes (TabView.tsx:296-307), so icons, focusedIcons, items and iconSizes (deps [..., trimmedRoutes]) are recomputed on every render, resolvedIconAssets/resolvedFocusedIconAssets re-run Image.resolveAssetSource for every tab, a fresh items array is handed to the native component each commit, and the route objects passed to renderScene, getLazy, getFreezeOnBlur, getSceneStyle and the SceneMap scenes are new identities — so renderScene({route, jumpTo}) receives a different route object each render, defeating React.memo/useMemo in screens keyed on the route. Since tabBarIconSize is static for the life of a screen, the copy achieves nothing but churn: any parent re-render of the navigator (state/param change, descriptor change, tabPress emit) now re-diffs the whole tab prop set.

Suggested change
return (
export default function NativeBottomTabView({
state,
layoutDirection,
navigation,
descriptors,
tabBar,
...rest
}: Props) {
const navigationState = React.useMemo(
() => ({
...state,
routes: state.routes.map((route) => ({
...route,
iconSize: descriptors[route.key]?.options.tabBarIconSize,
})),
}),
[state, descriptors]
);
return (
<TabView
{...rest}
navigationState={navigationState}

Suggested change

Wrap the derived state in React.useMemo keyed on state and descriptors so the routes array and route objects keep a stable identity between renders, or (better) pass a getIconSize={({route}) => descriptors[route.key]?.options.tabBarIconSize} callback — already supported by the new getIconSize prop — instead of mutating the routes.

Details

Also raised here

Also raised at this location by swift-ios: navigationState is a fresh object with fresh route objects on every render, so trimmedRoutes no longer stays referentially stable — NativeBottomTabView now builds a brand-new routes array (and a brand-new object per route) inline in the JSX, which breaks the one referential invariant TabView has: trimmedRoutes = useMemo(..., [navigationState.routes]) (TabView.tsx:296-306) used to be stable because navigationState={state} forwarded react-navigation's stable state object. Now it recomputes on every render of the tab view, and since descriptors (and the inline getIcon/getIconSize closures) already change identity each render, that cascades into fresh icons, focusedIcons, items and iconSizes arrays and a full Image.resolveAssetSource pass every time a descendant of the tab bar re-renders — even when no option changed. …

persona: javascript, jvm, swift-ios, maintainability

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.

import com.google.android.material.transition.platform.MaterialFadeThrough
import kotlin.math.roundToInt

private data class DrawableCacheKey(val source: ImageSource, val size: Int)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor · Android: duplicate drawables per icon size, and every size variant is kept for the view's lifetime

DrawableCacheKey puts the resolved pixel size into the key (private data class DrawableCacheKey(val source: ImageSource, val size: Int)), and drawableCache is never evicted. A tab bar whose iconSizes change over its lifetime — e.g. a size driven by state (the new getIconSize/tabBarIconSize is re-evaluated on every render, and updateItems re-runs whenever items or sizes change) — accumulates one decoded Drawable per (icon, size) pair for as long as the view lives. With 5 tabs and a handful of sizes that is several full bitmaps held in drawableCache in addition to Coil's own memory cache.

Details

The same key change also defeats the cross-tab reuse the old Map<ImageSource, Drawable> provided: two tabs that use the same asset at the same size each got one Drawable before, and still share via the key, but the same asset at 20/32/40 is now decoded and retained three times. updateItems already posts work on every call, so add a size-keyed eviction (e.g. keep only the size currently requested per source) or clear drawableCache when the sizes change.

Also raised here

Also raised at this location by maintainability: The drawable cache key gained size, so the cache is now bounded by distinct (source, size) pairs instead of distinc… — The drawable cache key gained size, so the cache is now bounded by distinct (source, size) pairs instead of distinct sources, and nothing evicts it: resizing the same tabs through several sizes (a state-driven or animated tabBarIconSize, or route churn where a size moves between indexes) keeps every rendered variant alive for the life of the view, and a size derived from layout can take many values. Before this change the worst case was one drawable per icon source. Consider clearing the entries for a source when its size changes, or bucketing the size in the key.

persona: cpp, maintainability

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.

@@ -241,6 +246,9 @@ class ReactBottomNavigationView(context: Context) : LinearLayout(context) {
}

fun updateItems(items: MutableList<TabInfo>) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nit · val items = items.mapIndexed { ... } shadows the parameter and the items property

Inside fun updateItems(items: MutableList<TabInfo>) the new first statement declares a local val items whose own initializer reads the parameter, while the class also has a var items: MutableList<TabInfo>. All three resolve to different things in the same scope, so this.items, the parameter and the local differ after this line; it only reads correctly because the parameter is shadowed. Rename it (e.g. val resolvedItems) and assign items = resolvedItems explicitly.

Suggested change
fun updateItems(items: MutableList<TabInfo>) {
fun updateItems(newItems: MutableList<TabInfo>) {
val items = newItems.mapIndexed { index, item ->
item.copy(iconSize = iconSizes.getOrNull(index)?.takeIf { it > 0 })
}.toMutableList()
Details

Also raised here

Also raised at this location by jvm: Local val items shadows the updateItems parameter and re-reads the mutable property from setIconSizes — fun updateItems(items: MutableList<TabInfo>) { val items = items.mapIndexed { ... } } shadows the parameter with a same-named local, which only compiles because the initializer resolves to the parameter; setIconSizes then calls updateItems(items) on the already-decorated property, so the mapping is applied to its own output and onConfigurationChanged does the same. Renaming the local (e.g. resolvedItems) makes the data flow explicit and keeps items.size < this.items.size from reading one list while this.items = items writes another.

Also raised at this location by maintainability: Inside updateItems this now reads val items = items.mapIndexed { … }, so within one function items means three di… — Inside updateItems this now reads val items = items.mapIndexed { … }, so within one function items means three different things depending on position: the parameter, the size-augmented local, and the property (this.items, used one line below in items.size < this.items.size). Kotlin only warns about the shadowing, so it compiles, but the next reader has to work out which list each read refers to, and the property passed in by setIconSizes (updateItems(items)) is immediately re-derived. Renaming the local (e.g. sizedItems) and assigning the property from it keeps the intent obvious. The same applies to iconSizePx, which is a pixel value in some paths and the Material default in others — a name like iconSizePxOrDefault would say so.

persona: swift-ios, jvm, maintainability

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.

@@ -127,6 +127,14 @@ - (void)updateProps:(Props::Shared const &)props oldProps:(Props::Shared const &
_tabViewProvider.itemsData = convertItemsToArray(newViewProps.items);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nit · iOS: itemsData is assigned before iconSizes, so updateItems() runs once with the stale sizes array

In updateProps:oldProps: (line 127) itemsData is assigned before iconSizes (line 135). itemsData's didSet runs updateItems() first, which re-applies the previous self.iconSizes array to the freshly created TabInfo objects (the itemsData pass reads iconSizes[safe: index]), so the tab bar renders the new items with the previous icons/sizes for one render cycle; the correct sizes are only applied when the iconSizes block runs moments later, and loadIcons may then be issued twice. Swapping the two blocks (sizes first, items second) makes the single updateItems() pass see both new inputs at once, removes the redundant reload, and removes the stale-size intermediate state. It also avoids the case where the route order changes: updateItems maps iconSizes positionally, so between the two assignments every tab can carry a size that belongs to a different route.

Suggested change
_tabViewProvider.itemsData = convertItemsToArray(newViewProps.items);
if (oldViewProps.iconSizes != newViewProps.iconSizes) {
NSMutableArray<NSNumber *> *iconSizes = [NSMutableArray arrayWithCapacity:newViewProps.iconSizes.size()];
for (const auto iconSize : newViewProps.iconSizes) {
[iconSizes addObject:@(iconSize)];
}
_tabViewProvider.iconSizes = iconSizes;
}
if (oldViewProps.items != newViewProps.items) {
_tabViewProvider.itemsData = convertItemsToArray(newViewProps.items);
}

Suggested change

Assign _tabViewProvider.iconSizes before _tabViewProvider.itemsData in updateProps (or gate updateItems() so it runs once per props update) so the new items are built with the new sizes in a single pass.

Details

persona: jvm

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.

@@ -237,6 +245,7 @@ - (void)updateProps:(Props::Shared const &)props oldProps:(Props::Shared const &
sfSymbol:RCTNSStringFromStringNilIfEmpty(item.sfSymbol)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nit · iconSize is always passed as a constant from the prop conversion on both platforms

The new plumbing hardcodes the value instead of reading it from the prop it sits next to: convertItemsToArray passes iconSize:nil (RCTTabViewComponentView.mm:248) and Android's setItems passes iconSize = null (RCTTabViewManager.kt:109), because the value is carried in the parallel iconSizes array (iconSizes?: ReadonlyArray<Double>). That works only while two invariants hold outside this code: iconSizes is delivered in the same commit as items with the same index alignment, and its length is at least items.count. TabView.tsx keeps them aligned today (both derived from trimmedRoutes, TabView.tsx:343-395), so nothing is wrong at the moment, but an item created before its sizes arrive (or after a truncation) silently gets the platform default with no warning — and on Android a getDouble on a null/undefined entry throws rather than falling back, so an out-of-range/missing array slot fails loudly while the iOS equivalent fails quietly.

Suggested change

Prefer carrying iconSize inside the items struct (like activeTintColor and iconRenderingMode already are), which removes the index-alignment requirement and the SwiftPM bridge change entirely; if the parallel array stays, document the alignment requirement where iconSizes is declared in TabViewNativeComponent.ts.

Details

persona: 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.

return CGSize(width: value, height: value)
}

private func updateItems() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nit · updateItems() reloads icons on a size diff only, so a size change arriving with a new icon source in one commit can resize the old image

TabViewProvider.updateItems() (TabViewProvider.swift:337-351) decides whether to re-run loadIcons by comparing previousIconSizes != itemsData.map(\.iconSize), and the reload is driven entirely off props.items, while the new image sources arrive through the separate icons/focusedIcons didSet observers. Fabric assigns several props in one updateProps pass, so a commit that changes both the icon source and the size for the same index can run loadIcons(icons, ...) from updateItems() with the sources icons still holds — i.e. resize and cache the previous image at the new size — before the icons observer fires with the new sources. In practice both observers fire in the same pass and the icons observer re-resizes with the correct source, so the wrong result is only transient and I could not demonstrate a lasting failure; the fragility is that the correctness rests on the observer order in -[RCTTabViewComponentView updateProps:oldProps:] (items → iconSizes → icons → focusedIcons, RCTTabViewComponentView.mm:125-158) rather than on anything in updateItems().

Suggested change

Make the icon reload driven by the sources rather than by a size diff computed from a different prop (e.g. key the cached resized image on (source, size) like the Android DrawableCacheKey does), so the ordering of prop assignments in the Objective-C updateProps cannot influence which image ends up resized.

Details

persona: javascript

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.

}
}

#if os(iOS)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nit · iOS per-tab sizing depends on UIImage alignment rects being honored by UITabBarItem

Outside the experimental_bakedTintColors path, the only thing that makes a resized icon render at a non-default size in the bar is alignedIcon, which calls image.withAlignmentRectInsets with (size - 27)/2 on each edge; the icon bitmap itself is already iconSize points and nothing else (no UITabBarItem image-size API, no appearance iconSize) is set in the non-baked path. UITabBarItem/UITabBar layout is not documented as using a bar-button item image's alignment rect, so if UIKit ignores it there, a 40 pt icon would be drawn at its bitmap size only incidentally (or cropped/scaled by the bar's own icon metrics) rather than because the inset did anything — while with experimental_bakedTintColors the size is applied explicitly via makeTabBarItemImage(iconSize:). Worth a comment or an explicit mechanism so the mechanism does not silently depend on undocumented UIKit behaviour; the PR's own "label about 4 pt lower" limitation in the baked path hints the two paths are aligned by different means.

Suggested change

Confirm on the oldest supported iOS version that UITabBarItem honours image alignment rects for icon sizing, or drive the size through a mechanism that is documented (bake the icon into the target-size canvas as the baked-tint path already does) and note the dependency in a comment.

Details

persona: javascript

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tab icon size is hard-coded to 27x27pt with no way to override it

2 participants