Skip to content

Commit c2fe3a8

Browse files
chrismt0maboro
andauthored
fix: use path-based menuId to prevent collisions in nested submenus (#3762)
Closes: #3606 ## Summary - `prepareMenu` passes `menuIndex` as `index` when recursing into submenus, losing the original bar button item index - This causes `menuId` collisions between actions in different submenus and sibling toolbar menus at the same position - Fix threads a `path` string through recursion to track nesting depth, while preserving the original bar button item index ## Problem Given two toolbar menu buttons — a filter menu at bar button index 1 containing submenus, and a sort menu at bar button index 2: ``` Toolbar: [+] button (index 0) [filter] menu (index 1) ← contains Location, Item Type, Quality submenus [sort] menu (index 2) ← contains Title, Gear Type, Date Added actions ``` When `prepareMenu` recurses into the Quality submenu (at `menuIndex` 2 inside the filter menu), it calls: ```ts prepareMenu(menuItem, menuIndex, side) // menuIndex=2, replacing index=1 ``` The first action inside Quality then gets `menuId: "0-2-right"`, which collides with the first action in the sort menu (`"0-2-right"`). Selecting a quality filter fires the sort handler instead. ## Fix Add a `path` parameter that tracks position through nesting, while keeping the original bar button `index`: ```ts const prepareMenu = ( menu, index, side, path = '', // NEW ) => { return { ...menu, items: menu.items.map((menuItem, menuIndex) => { const currentPath = path ? `${path}.${menuIndex}` : `${menuIndex}`; // ... if (menuItem.type === 'submenu') { return { ...menuItem, ...prepareMenu(menuItem, index, side, currentPath), // ^^^^^ preserved (was menuIndex) }; } return { ...menuItem, menuId: `${currentPath}-${index}-${side}`, // ^^^^^^^^^^^^ path-based (was menuIndex) }; }), }; }; ``` **Before:** Quality "All" = `0-2-right` (collides with Sort "Title" = `0-2-right`) **After:** Quality "All" = `2.0-1-right`, Sort "Title" = `0-2-right` (unique) ## Before & after - visual documentation | Before | After | | --- | --- | | <video src="https://github.com/user-attachments/assets/44420c7e-7850-45be-b4b2-558c38da7b1a" /> | <video src="https://github.com/user-attachments/assets/a0deac60-ac7e-4197-91fe-6943a875d06c" /> | ## Verified - Toolbar with multiple menu buttons — actions in each menu fire the correct handler - Nested submenus (inline sections) within a toolbar menu — no cross-talk with sibling toolbar menus - Single menu button with no submenus — no regression (path defaults to menuIndex) --------- Co-authored-by: Tomasz Boroń <tomasz.boron@swmansion.com>
1 parent d4f3d71 commit c2fe3a8

3 files changed

Lines changed: 141 additions & 2 deletions

File tree

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
import React, { useEffect } from 'react';
2+
import { Alert, StyleSheet, Text, View } from 'react-native';
3+
import { NavigationContainer, ParamListBase } from '@react-navigation/native';
4+
import {
5+
createNativeStackNavigator,
6+
NativeStackNavigationProp,
7+
} from '@react-navigation/native-stack';
8+
9+
type RouteParamList = {
10+
Home: undefined;
11+
};
12+
13+
type StackNavigationProp = {
14+
navigation: NativeStackNavigationProp<ParamListBase>;
15+
};
16+
17+
const Stack = createNativeStackNavigator<RouteParamList>();
18+
19+
function HomeScreen({ navigation }: StackNavigationProp) {
20+
useEffect(() => {
21+
navigation.setOptions({
22+
unstable_headerRightItems: () => [
23+
{
24+
type: 'menu',
25+
label: 'Menu 1',
26+
menu: {
27+
items: [
28+
{
29+
type: 'action',
30+
label: 'Action 1',
31+
onPress: () => Alert.alert('Menu 1', 'Action 1'),
32+
},
33+
{
34+
type: 'submenu',
35+
label: 'Submenu 1',
36+
items: [
37+
{
38+
type: 'action',
39+
label: 'Action 1',
40+
onPress: () => Alert.alert('Submenu 1', 'Action 1'),
41+
},
42+
{
43+
type: 'action',
44+
label: 'Action 2',
45+
onPress: () => Alert.alert('Submenu 1', 'Action 2'),
46+
},
47+
],
48+
},
49+
],
50+
},
51+
},
52+
{
53+
type: 'menu',
54+
label: 'Menu 2',
55+
menu: {
56+
items: [
57+
{
58+
type: 'action',
59+
label: 'Action 1',
60+
onPress: () => Alert.alert('Menu 2', 'Action 1'),
61+
},
62+
{
63+
type: 'action',
64+
label: 'Action 2',
65+
onPress: () => Alert.alert('Menu 2', 'Action 2'),
66+
},
67+
],
68+
},
69+
},
70+
],
71+
});
72+
}, [navigation]);
73+
74+
return (
75+
<View style={styles.container}>
76+
<Text style={styles.title}>
77+
menuId collision: submenu vs top-level menu
78+
</Text>
79+
80+
<Text style={styles.description}>
81+
<Text style={styles.bold}>Steps to reproduce{'\n'}</Text>
82+
1. Tap Menu 1{'\n'}
83+
2. Open Submenu 1{'\n'}
84+
3. Select Action 1{'\n\n'}
85+
<Text style={styles.pass}>
86+
<Text style={styles.bold}>Expected alert:{'\n'}</Text>
87+
Submenu 1{'\n'}
88+
Action 1{'\n\n'}
89+
</Text>
90+
<Text style={styles.bug}>
91+
<Text style={styles.bold}>Bug alert:{'\n'}</Text>
92+
Menu 1{'\n'}
93+
Action 1{'\n'}
94+
</Text>
95+
</Text>
96+
</View>
97+
);
98+
}
99+
100+
export default function App() {
101+
return (
102+
<NavigationContainer>
103+
<Stack.Navigator>
104+
<Stack.Screen name="Home" component={HomeScreen} />
105+
</Stack.Navigator>
106+
</NavigationContainer>
107+
);
108+
}
109+
110+
const styles = StyleSheet.create({
111+
container: {
112+
flex: 1,
113+
justifyContent: 'center',
114+
alignItems: 'center',
115+
padding: 20,
116+
},
117+
title: {
118+
fontSize: 18,
119+
fontWeight: 'bold',
120+
marginBottom: 16,
121+
textAlign: 'center',
122+
},
123+
description: {
124+
fontSize: 14,
125+
lineHeight: 22,
126+
},
127+
pass: {
128+
color: 'green',
129+
},
130+
bug: {
131+
color: 'red',
132+
},
133+
bold: {
134+
fontWeight: 'bold',
135+
},
136+
});

apps/src/tests/issue-tests/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,7 @@ export { default as Test3566 } from './Test3566';
179179
export { default as Test3568 } from './Test3568';
180180
export { default as Test3576 } from './Test3576';
181181
export { default as Test3596 } from './Test3596';
182+
export { default as Test3606 } from './Test3606';
182183
export { default as Test3611 } from './Test3611';
183184
export { default as Test3617 } from './Test3617';
184185
export { default as Test3636 } from './Test3636';

src/components/helpers/prepareHeaderBarButtonItems.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,12 @@ const prepareMenu = (
88
menu: HeaderBarButtonItemWithMenu['menu'],
99
index: number,
1010
side: 'left' | 'right',
11+
path: string = '',
1112
): HeaderBarButtonItemWithMenu['menu'] => {
1213
return {
1314
...menu,
1415
items: menu.items.map((menuItem, menuIndex) => {
16+
const currentPath = path ? `${path}.${menuIndex}` : `${menuIndex}`;
1517
const iconType = menuItem.icon?.type;
1618
const sfSymbolName =
1719
iconType === 'sfSymbol' ? menuItem.icon?.name : undefined;
@@ -32,7 +34,7 @@ const prepareMenu = (
3234
xcassetName,
3335
imageSource,
3436
templateSource,
35-
...prepareMenu(menuItem, menuIndex, side),
37+
...prepareMenu(menuItem, index, side, currentPath),
3638
};
3739
}
3840
return {
@@ -41,7 +43,7 @@ const prepareMenu = (
4143
xcassetName,
4244
imageSource,
4345
templateSource,
44-
menuId: `${menuIndex}-${index}-${side}`,
46+
menuId: `${currentPath}-${index}-${side}`,
4547
};
4648
}),
4749
};

0 commit comments

Comments
 (0)