Use useIsFocused may lead to app crash

[badeggg]

Current behavior

Use useIsFocused may lead to app crash

The causing code lines are

react-navigation/packages/core/src/useIsFocused.tsx

Lines 16 to 23 in 52a3234

	if (isFocused !== valueToReturn) {
	// If the value has changed since the last render, we need to update it.
	// This could happen if we missed an update from the event listeners during re-render.
	// React will process this update immediately, so the old subscription value won't be committed.
	// It is still nice to avoid returning a mismatched value though, so let's override the return value.
	// This is the same logic as in https://github.com/facebook/react/tree/master/packages/use-subscription
	setIsFocused(valueToReturn);
	}

I must say that if we write code lines like:

  const [foo, setFoo] = useState(true);

  ...

  setFoo(true);

inside a react function component, the function component will loop forever. This is because:

If you update a State Hook to the same value as the current state, react may still need to render that specific component again before bailing out.

Please refer react doc here or this stack overflow answer. So code

react-navigation/packages/core/src/useIsFocused.tsx

Lines 16 to 23 in 52a3234

	if (isFocused !== valueToReturn) {
	// If the value has changed since the last render, we need to update it.
	// This could happen if we missed an update from the event listeners during re-render.
	// React will process this update immediately, so the old subscription value won't be committed.
	// It is still nice to avoid returning a mismatched value though, so let's override the return value.
	// This is the same logic as in https://github.com/facebook/react/tree/master/packages/use-subscription
	setIsFocused(valueToReturn);
	}

may cause a loop.

Check this code sandbox demo to verify what I'm saying.

A loop case could be: Expensify/App#42824 (comment), or Expensify/App#42916

It's kind difficult to make a minimal exact reproduction(I tried literally one day), but I think what I am explaining above is convincible enough.

Expected behavior

use useIsFocused must not lead to app crash

Reproduction

Loop cases Expensify/App#42824 and Expensify/App#42916, will be fixed by applied those changes

Code sandbox demo to verify the bad code style: https://codesandbox.io/p/sandbox/young-platform-phwfhv?file=%2Fsrc%2FApp.js%3A22%2C29

To remove label "needs repro" https://github.com/badeggg/expensify-app

Platform

• Android
• iOS
• Web
• Windows
• MacOS

Packages

• @react-navigation/bottom-tabs
• @react-navigation/drawer
• @react-navigation/material-top-tabs
• @react-navigation/stack
• @react-navigation/native-stack
• react-native-tab-view

Environment

• I've removed the packages that I don't use

package	version
@react-navigation/native	6.1.12
@react-navigation/material-top-tabs	6.6.3
@react-navigation/stack	6.3.29
react-native-safe-area-context	4.8.2
react-native-screens	3.30.1
react-native-gesture-handler	2.14.1
react-native-reanimated	3.8.1
react-native-tab-view	3.5.2
react-native-pager-view	6.2.3
react-native	0.73.4
expo	50.0.4
node	20.13.0
npm or yarn	npm 10.5.2

[github-actions]

Hey @badeggg! Thanks for opening the issue. It seems that the issue doesn't contain a link to a repro.

The best way to get attention to your issue is to provide an easy way for a developer to reproduce the issue.

You can provide a repro using any of the following:

• Expo Snack
• TypeScript Playground
• GitHub repo under your username

A snack link is preferred since it's the easiest way to both create and share a repro. If it's not possible to create a repro using a snack, link to a GitHub repo under your username is a good alternative. Don't link to a branch or specific file etc. as it won't be detected.

Try to keep the repro as small as possible by narrowing down the minimal amount of code needed to reproduce the issue. Don't link to your entire project or a project containing code unrelated to the issue. See "How to create a Minimal, Reproducible Example" for more information.

You can edit your original issue to include a link to the repro, or leave it as a comment. The issue will be closed automatically after a while if you don't provide a repro.

[github-actions]

The versions mentioned in the issue for the following packages differ from the latest versions on npm:

• @react-navigation/native (found: 6.1.12, latest: 6.1.17)
• @react-navigation/material-top-tabs (found: 6.6.3, latest: 6.6.13)

Can you verify that the issue still exists after upgrading to the latest versions of these packages?

[badeggg]

Think about change

react-navigation/packages/core/src/useIsFocused.tsx

Lines 16 to 23 in 52a3234

	if (isFocused !== valueToReturn) {
	// If the value has changed since the last render, we need to update it.
	// This could happen if we missed an update from the event listeners during re-render.
	// React will process this update immediately, so the old subscription value won't be committed.
	// It is still nice to avoid returning a mismatched value though, so let's override the return value.
	// This is the same logic as in https://github.com/facebook/react/tree/master/packages/use-subscription
	setIsFocused(valueToReturn);
	}

to:

  React.useEffect(() => {
      if (isFocused !== valueToReturn) {
        // If the value has changed since the last render, we need to update it.
        // This could happen if we missed an update from the event listeners during re-render.
        // React will process this update immediately, so the old subscription value won't be committed.
        // It is still nice to avoid returning a mismatched value though, so let's override the return value.
        // This is the same logic as in https://github.com/facebook/react/tree/master/packages/use-subscription
        setIsFocused(valueToReturn);
      }
  }, [isFocused, valueToReturn]);

There must be a re-render leads to if (isFocused !== valueToReturn) { be checked and setIsFocused(valueToReturn); be executed is condition met, the same re-render will definitely leads to

  React.useEffect(() => {
      if (isFocused !== valueToReturn) {
        // If the value has changed since the last render, we need to update it.
        // This could happen if we missed an update from the event listeners during re-render.
        // React will process this update immediately, so the old subscription value won't be committed.
        // It is still nice to avoid returning a mismatched value though, so let's override the return value.
        // This is the same logic as in https://github.com/facebook/react/tree/master/packages/use-subscription
        setIsFocused(valueToReturn);
      }
  }, [isFocused, valueToReturn]);

be checked. If conditionisFocused !== valueToReturn is true, there must be a value change of isFocused or valueToReturn or both. Which leads if (isFocused !== valueToReturn) { be checked and setIsFocused(valueToReturn); be executed is condition met.

So after the change, logic will be the same, except that set an identical value with previous value (setIsFocused(valueToReturn)) is not leading to a loop check of if (isFocused !== valueToReturn) {, hence avoid loop

[badeggg]

Think about simply delete

react-navigation/packages/core/src/useIsFocused.tsx

Lines 16 to 23 in 52a3234

	if (isFocused !== valueToReturn) {
	// If the value has changed since the last render, we need to update it.
	// This could happen if we missed an update from the event listeners during re-render.
	// React will process this update immediately, so the old subscription value won't be committed.
	// It is still nice to avoid returning a mismatched value though, so let's override the return value.
	// This is the same logic as in https://github.com/facebook/react/tree/master/packages/use-subscription
	setIsFocused(valueToReturn);
	}

This code block is for reason that It is still nice to avoid returning a mismatched value though, so let's override the return value. But, guess what, we are returning right value here:

react-navigation/packages/core/src/useIsFocused.tsx

Line 42 in 52a3234

return valueToReturn;

The only effect of setIsFocused(valueToReturn); is it will trigger a self-component re-render, which is not needed.

So maybe we should simple delete this code block, if I am not missing something.

[satya164]

Codesandbox link doesn't work.

[badeggg]

@satya164 Hi, please try codesandbox link again. I forgot to change 'who can access'.
https://codesandbox.io/p/sandbox/young-platform-phwfhv?file=%2Fsrc%2FApp.js%3A15%2C24

[satya164]

The codesandbox doesn't show an issue with the logic in useIsFocused.

In the codesandbox, you're updating the state if (count > 1) - which means setCount will be called even when count is already 1 and state doesn't need to change, which is different logic from useIsFocused where we only update the state if the value doesn't match the state (isFocused !== valueToReturn). If you update your sandbox to match the logic, e.g. (count !== 1), then there's no infinite loop.

Also please provide a repro using where using the hook from React Navigation crashes your code, not an approximation of the logic we have - which may not match what's actually happening.

[badeggg]

Also please provide a repro using where using the hook from React Navigation crashes your code, not an approximation of the logic we have - which may not match what's actually happening.

I'll try again to provide an exact minimal repro.

The code snippet in codesandbox is trying to say, we should not call setState directly in function body.

Anyway, the loop is like this:

1. setIsFocused(valueToReturn); trigger a re-render
2. When re-rendering, condition if (isFocused !== valueToReturn) "may" met, (I know the key part is "may", I'm trying to find out how)
3. setIsFocused(valueToReturn); trigger a re-render
4. Loop...

[satya164]

The code snippet in codesandbox is trying to say, we should not call setState directly in function body.

It needs to provide a valid reason to say that. The pattern is also used in React docs https://react.dev/reference/react/useState#storing-information-from-previous-renders

I know the key part is "may"

If it doesn't satisfy the condition and does a state update, then the next render with the new state will satisfy the condition. If it doesn't then there's a different problem.

setIsFocused(valueToReturn); trigger a re-render
Loop...

It'll only loop if the setIsFocused didn't actually update the state. If setIsFocused(valueToReturn) actually updated the state and caused a re-render, the condition should be satisfied in the next render and there won't be another state update - unless navigation.isFocused() returns a different value every time - which would be a different issue.

[badeggg]

@satya164 Hi, do you know in what situation, condition if (isFocused !== valueToReturn) will be true? I'm trying to make a minimal exact repro, but is struggling to make the first trigger if (isFocused !== valueToReturn).

I noticed the comment line, but can not find a way to trigger.

react-navigation/packages/core/src/useIsFocused.tsx

Line 18 in 52a3234

// This could happen if we missed an update from the event listeners during re-render.

[satya164]

effects run asynchronously. it's possible that there was a state change/navigation before the effect could run. however, it's an edge case and the logic handles that. i don't know how to trigger this edge case manually.

but according to your issue, it becomes true in your app if the setIsFocused is being called. so you already have a repro and need to find whatever is happening in your app that causes it.

[badeggg]

Yes, I do notice the Expensify App is experiencing that condition met, e.g. this issue and this issue. The later one only happen on physical iOS. Just, 'expensify app' is a big app, it's kind difficult to find out what is triggering.

In the two issues I just mentioned, if I wrap

  if (isFocused !== valueToReturn) {
    // If the value has changed since the last render, we need to update it.
    // This could happen if we missed an update from the event listeners during re-render.
    // React will process this update immediately, so the old subscription value won't be committed.
    // It is still nice to avoid returning a mismatched value though, so let's override the return value.
    // This is the same logic as in https://github.com/facebook/react/tree/master/packages/use-subscription
    setIsFocused(valueToReturn);
  }

into useEffect like this comment say, the bug disappears.

That's why I reported this issue. It's rational I suspect this code block.

Still trying to make a minimal repro...

[github-actions]

Hello 👋, this issue has been open for more than a month without a repro or any activity. If the issue is still present in the latest version, please provide a repro or leave a comment within 7 days to keep it open, otherwise it will be closed automatically. If you found a solution or workaround for the issue, please comment here for others to find. If this issue is critical for you, please consider sending a pull request to fix it.