Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Navigation twice on the first screen of a modal (only in production app) #847

Closed
wcastand opened this issue Aug 18, 2023 · 7 comments
Closed
Assignees

Comments

@wcastand
Copy link

wcastand commented Aug 18, 2023

Which package manager are you using? (Yarn is recommended)

yarn

Summary

As you can see in the video, when i open a modal that have a stack inside, i get a bug where it navigate twice to the first screen. It only happens in production/built app. I can't reproduce the problem locally to try and debug it so a bit stuck.

RPReplay_Final1692348709.mov

the structure is :

app/
  _layout (Stack)
  accounts
    wallet
      create (modal you see in video)
        _layout (Stack)
        amount
        name (the screen that appear twice)

app option layout

const screenAppOption = {
	title: "",
	headerBackTitleVisible: false,
	headerShadowVisible: false,
	// theme is not init yet so directly accessing color, don't do that elsewhere
	headerTintColor: color["light.primary.900"],
	contentStyle: { backgroundColor: color["light.white"] },
}

the modal has this options:

const options = {presentation: "modal", headerShown: false}

and the Stack inside the modal:

const options = {
  title: "",
			headerBackTitleVisible: false,
			headerShadowVisible: false,
			headerTintColor,
			headerTitle: ({ children }: PropsWithChildren<{}>) => <Text variant="bodyBold">{children}</Text>,
			headerRight: () =>
				Platform.OS === "ios" ? (
					<Pressable
						onPress={() =>
							target
								? target()
								: router.canGoBack()
								? router.back()
								: router.replace(isConnected ? "/home" : "/welcome")
						}
					>
						<AppleClose />
					</Pressable>
				) : undefined,
			contentStyle: { backgroundColor },
}

Minimal reproducible example

can't reproduce in local dev so not sure what to show here. still is an issue tho :/

potentially linked to #838

UPDATE:

still trying to understand but managed to get something locally by removing the modal presentation.

I tried to remove the screenOptions on the Stack inside the modal (bug still happens)
if i use a Slot instead of Stack, it seems to fix the issue (at this point there is basically no more Stack inside the Stack since no modal and it's Slot so useless _layout in accounts/wallets/create

from what i can understand locally (not much lol)
it looks like the screen accounts/wallets/create/name is mounted once, then the stack accounts/wallets/create/_layout get mounted which trigger to mount accounts/wallets/create/name again in the stack, which explain the double nav happening.

in local, only happen if i remove the presentation: modal but on production app, the bug appear with the modal presentation on.

@EGurney
Copy link

EGurney commented Aug 21, 2023

I also have this same issue. Same exact thing you are experiencing where a modal sub route is mounted twice, which causes the user to see the initial layout correctly and then see the "slide-in" transition to the same screen.

@fluid-design-io
Copy link

Also happens to me as well, the double mounting applies to both Android and iOS.

@devoren
Copy link

devoren commented Aug 24, 2023

#320
I guess it doesn't matter if it's modal, try to push screen usingaccounts/wallets/create instead accounts/wallets/create/name

UPD:
Yes, it's _layout's fault, if I remove it, the screen is mounted once

@EvanBacon
Copy link
Contributor

Please provide a minimal reproducible demo so I can fix.

@EvanBacon EvanBacon self-assigned this Aug 28, 2023
EvanBacon added a commit to expo/expo that referenced this issue Aug 28, 2023
# Why

Prevent double renders by cloning state to avoid leaking state between
functions.

- fix expo/router#838 
- fix expo/router#733
- fix expo/router#320
- The issue expo/router#320 has multiple
different things linked, but the original case appears to be fixed.
- possibly also addresses expo/router#847

<!--
Please describe the motivation for this PR, and link to relevant GitHub
issues, forums posts, or feature requests.
-->

# How

- Prevent mutating the input state to avoid invalidating the nested
state.

# Test Plan

- The testing library doesn't seem to support this case. @marklawlor has
been tasked with ensuring the original branch can detect the error
https://github.com/expo/expo/compare/%40evanbacon/router/fix-838
Just in case the testing library isn't fixed, I ran locally with:

- `app/_layout.js`, `app/(a)/_layout.js`, `app/b/_layout.js`
```js
import { Slot } from "expo-router";
export default function RootLayout() {
  return <Slot />
}
```
- `app/(a)/index.js`
```js
import { router, useNavigation } from "expo-router";
import {  View } from "react-native";

export default function App() {
  // const navigation = useNavigation();

  setTimeout(() => {
    router.push("/b");
    // navigation.push("b");
  });
  return (
    <View />
  );
}
```
- `app/b/index.js`
```js
import { usePathname } from 'expo-router';
import { Text, View } from 'react-native';

let i = 0;
export default function Page() {
  const path = usePathname();
  i++;
  return (
    <View style={{ flex: 1, backgroundColor: i > 1 ? "red" : "white" }}>
      <Text>Path: {path}</Text>
    </View>
  );
}
```



<!--
Please describe how you tested this change and how a reviewer could
reproduce your test, especially if this PR does not include automated
tests! If possible, please also provide terminal output and/or
screenshots demonstrating your test/reproduction.
-->

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).

---------

Co-authored-by: Expo Bot <[email protected]>
@EvanBacon
Copy link
Contributor

EvanBacon commented Aug 28, 2023

A possible fix was published in [email protected]. Let me know if this resolves the issue for you.

@EvanBacon
Copy link
Contributor

I can confirm this is fixed in 2.0.4

@akpor-kofi
Copy link

please this issue still persists on v3.5.23

DavidAmyot pushed a commit to Villeco-inc/expo-router that referenced this issue Oct 16, 2024
# Why

Prevent double renders by cloning state to avoid leaking state between
functions.

- fix expo/router#838 
- fix expo/router#733
- fix expo/router#320
- The issue expo/router#320 has multiple
different things linked, but the original case appears to be fixed.
- possibly also addresses expo/router#847

<!--
Please describe the motivation for this PR, and link to relevant GitHub
issues, forums posts, or feature requests.
-->

# How

- Prevent mutating the input state to avoid invalidating the nested
state.

# Test Plan

- The testing library doesn't seem to support this case. @marklawlor has
been tasked with ensuring the original branch can detect the error
https://github.com/expo/expo/compare/%40evanbacon/router/fix-838
Just in case the testing library isn't fixed, I ran locally with:

- `app/_layout.js`, `app/(a)/_layout.js`, `app/b/_layout.js`
```js
import { Slot } from "expo-router";
export default function RootLayout() {
  return <Slot />
}
```
- `app/(a)/index.js`
```js
import { router, useNavigation } from "expo-router";
import {  View } from "react-native";

export default function App() {
  // const navigation = useNavigation();

  setTimeout(() => {
    router.push("/b");
    // navigation.push("b");
  });
  return (
    <View />
  );
}
```
- `app/b/index.js`
```js
import { usePathname } from 'expo-router';
import { Text, View } from 'react-native';

let i = 0;
export default function Page() {
  const path = usePathname();
  i++;
  return (
    <View style={{ flex: 1, backgroundColor: i > 1 ? "red" : "white" }}>
      <Text>Path: {path}</Text>
    </View>
  );
}
```



<!--
Please describe how you tested this change and how a reviewer could
reproduce your test, especially if this PR does not include automated
tests! If possible, please also provide terminal output and/or
screenshots demonstrating your test/reproduction.
-->

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).

---------

Co-authored-by: Expo Bot <[email protected]>
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

No branches or pull requests

6 participants