[HOLD for payment 2024-06-20] [$250] Start chat - Uh-oh something went wrong displayed when tapping room and going back

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.77-0
Reproducible in staging?: Y
Reproducible in production?: Y
Found when executing PR : #42257
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Open the app and log in
2. Create a workspace if you do not have any
3. Tap FAB > Start chat
4. Select #room
5. If workspace is not preselected, select a workspace
6. After keyboard comes out, go back to the LHN

Expected Result:

User is returned to the LHN

Actual Result:

The "Uh-oh something went wrong" screen is displayed

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6495508_1717013855356.IMG_7234.mp4

ios logs.txt

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012c83bbe79ba0af18
• Upwork Job ID: 1797718438231285760
• Last Price Increase: 2024-06-03
• Automatic offers:
• badeggg | Contributor | 102628382

Issue Owner

Current Issue Owner: @johncschuster

[melvin-bot]

Triggered auto assignment to @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[izarutskaya]

We think this issue might be related to the #vip-vsb

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

App crashes when going back from new room page while keyboard is showing.

What is the root cause of that problem?

I can't find the real root cause of the issue, but the crash is because we are calling the set state "infinitely" reaching the max update.

App/src/hooks/useAutoFocusInput.ts

Lines 50 to 53 in f26c9f5

	const inputCallbackRef = (ref: TextInput | null) => {
	inputRef.current = ref;
	setIsInputInitialized(true);
	};

inputCallbackRef will be called when the component is mounted, rerendered, or unmounted. In our case, it rerenders infinitely.

This only happens if the keyboard is showing while we close the page. Also, if we remove the useIsFocused hook, then the issue is gone.

App/src/pages/workspace/WorkspaceNewRoomPage.tsx

Line 64 in f26c9f5

const isFocused = useIsFocused();

What changes do you think we should make in order to solve the problem?

If the ref is already set, return early.

App/src/hooks/useAutoFocusInput.ts

Lines 50 to 53 in f26c9f5

	const inputCallbackRef = (ref: TextInput | null) => {
	inputRef.current = ref;
	setIsInputInitialized(true);
	};

if (inputRef.current) {
    return;
}

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~012c83bbe79ba0af18

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @jayeshmangwani (External)

[jayeshmangwani]

@bernhardoj Your proposal will solve the crash tested, but I am trying to understand why it is happening for the only WorkspaceNewRoomPage, I am testing different variations on the other pages where we are using the inputCallbackRef, but not able to reproduce this crash for other pages, curious whats the cause for the WorkspaceNewRoomPage only and not others.

[dominictb]

Bringing my proposal from #42916 since this has similar root causes

Proposal

Please re-state the problem that we are trying to solve in this issue.

Show error when access starts to chat 2nd time in off-mode

What is the root cause of that problem?

In

App/src/hooks/useAutoFocusInput.ts

Line 52 in 1d5f83d

setIsInputInitialized(true);

, the app is not checking that isInputInitialized is false first before setting it to true. So there're too many unnecessary rerendering of the hook that causes the crash.

What changes do you think we should make in order to solve the problem?

Check isInputInitialized is false first before setting it to true in

App/src/hooks/useAutoFocusInput.ts

Line 52 in 1d5f83d

setIsInputInitialized(true);

const inputCallbackRef = (ref: TextInput | null) => {
    inputRef.current = ref;

    if (!isInputInitialized) {
        setIsInputInitialized(true);
    }
};

This can be further polished by using useCallback for the inputCallbackRef

After the crash is fix, we'll face another issue: when trying to create the room the second time, the modal will be automatically dismissed, the root cause is in

App/src/pages/workspace/WorkspaceNewRoomPage.tsx

Line 129 in 1d5f83d

Report.clearNewRoomFormError();

, the app is clearing the form loading state only when it's mounted. When the loading state is initially true, it already causes the modal to be dismissed in here, before the "clear isLoading" useEffect finishes running.

This can be fixed by clearing the isLoading when the component is unmounted too (or can change to clearing when the screen is focused/loses focus)

useEffect(() => {
    Report.clearNewRoomFormError();

    return () => {
        Report.clearNewRoomFormError();
    }
}, []);

Or clearing whenever we call Navigation.dismissModal in the page should also be ok

What alternative solutions did you explore? (Optional)

For the crash issue, we could also early return here

App/src/hooks/useAutoFocusInput.ts

Line 50 in 1d5f83d

const inputCallbackRef = (ref: TextInput | null) => {

if the inputRef was already set

For the modal dismissing issue, we can remove this block and the related error clearing logic entirely. They were meant to support the use case that there's a duplicate room name error thrown from the back-end, but we have the front-end validation logic for that in

App/src/pages/workspace/WorkspaceNewRoomPage.tsx

Line 179 in 1d5f83d

} else if (ValidationUtils.isExistingRoomName(values.roomName, reports, values.policyID ?? '')) {

already so that block is no longer needed.

And we already have the dismissModal logic after the user creates the room here

App/src/libs/actions/Report.ts

Line 2102 in 1d5f83d

Navigation.dismissModalWithReport(policyReport);

[bernhardoj]

why it is happening for the only WorkspaceNewRoomPage, I am testing different variations on the other pages where we are using the inputCallbackRef, but not able to reproduce this crash for other pages

If you add useIsFocused usage on other page, then it will starts to crash as mentioned in my proposal, but I really don't understand either why the hook causes the infinite rendering issue.

[jayeshmangwani]

the app is not checking that isInputInitialized is false first before setting it to true. So there're too many unnecessary rerendering of the hook that causes the crash.

@dominictb main thing that I am not able to understand is why unnecessary rerendering happens at first place ?

[jayeshmangwani]

If you add useIsFocused usage on other page, then it will starts to crash as mentioned in my proposal, but I really don't understand either why the hook causes the infinite rendering issue.

yes that's confuses me, why it is not happening for only inputCallbackRef but happens if we have used the useIsFocused, so it brings me question do we really fixing the root cause here or we are just adding band aid to the real fix

[badeggg]

Seems it's related to react bailing out of a state update, and stack overflow here.

If you update a State Hook to the same value as the current state, React will bail out without rendering the children or firing effects.
Note that React may still need to render that specific component again before bailing out.

Also note that "React may still need to render that specific component again before bailing out." means running the render function one more time, not "render to the DOM one more time", so any unexpected side-effects, like enqueueing another state update are problematic. The entire function body of a function component is the render function.

But I'm still confused why useIsFocused is making a difference and why offline is making a difference.

[badeggg]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When offline, open new room page may cause app crashing

What is the root cause of that problem?

The apparent cause is this line is called indefinitely

App/src/hooks/useAutoFocusInput.ts

Line 52 in f26c9f5

setIsInputInitialized(true);

But the root cause is that a loop is running. setIsInputInitialized(true); is just a chain node of the loop. Before explain the loop, I must declare 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. Refer this stack overflow answer for details.

Similar code line happens in @react-navigation/core here in our project.

The loop of this issue is:

1. When offline, a page re-render is triggered, condition if (isFocused !== valueToReturn) { is met. I haven't dug too much about why offline can make this condition met. Since there are other cases that will make condition if (isFocused !== valueToReturn) { met, refer comment here
2. setIsFocused(valueToReturn); is executed
3. A re-render is triggered, mount event happened, setIsInputInitialized(true); is executed
4. A re-render is triggered. Condition if (isFocused !== valueToReturn) { is met. setIsFocused(valueToReturn); is executed
5. A re-render is triggered. setIsInputInitialized(true); is executed
6. ....

What changes do you think we should make in order to solve the problem?

We should patch package @react-navigation/core here 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]);

Besides, we should report a bug to @react-navigation, and we should change

App/src/hooks/useAutoFocusInput.ts

Line 52 in f26c9f5

setIsInputInitialized(true);

to

if (isInputInitialized) {
    return;
}
setIsInputInitialized(true);

like other proposals said for code robust reason.

What alternative solutions did you explore? (Optional)

N/A

[jayeshmangwani]

When offline, a page re-render is triggered

I don't agree with the root cause here as Issue can be reproduced online too, And if you found that issue is from the navigation package then raise an issue to the navigation repo with minimal reproduction steps/Or any outstanding issues that confirms issue is from the package ?

[badeggg]

@jayeshmangwani Hi, the expression "When offline, a page re-render is triggered" is actually proper for #42916, which has same root cause with this issue. Sorry for my oversight, I will amend my proposal later. Except part "When offline, a page re-render is triggered", the other part should be proper. Sorry again.

I will try to raise an issue to the navigation package repo with minimal reproduction steps tomorrow (if I have the bandwidth)

[badeggg]

To confirm my root cause explanation, after install package with npm install,

1. Open node_modules/@react-navigation/core/src/useIsFocused.tsx in a code editor, change

  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]);

2. Change

   App/src/pages/workspace/WorkspaceNewRoomPage.tsx

   Line 1 in f26c9f5

   import {useIsFocused} from '@react-navigation/core';

   
   to

import useIsFocused from '@react-navigation/core/src/useIsFocused';

3. npm run ios, you should notice the bug disappears

[jayeshmangwani]

Thanks for the investigation @badeggg , And yes I applied these changes to useIsFocused.tsx file and Issue disappeared, So I would agree that root cause for this issue can be from the @react-navigation/core, @badeggg Can you please raise the Issue Upstream to double check and also to see what will the side effect of the adding the useEffect for setIsFocused ?

@badeggg 's RCA makes sense here, we can go with the any of their suggestion, Either to fix upstream OR adding the check isInputInitialized in inputCallbackRef, OR can do both here.

Note: Proposal of @bernhardoj: Return early if inputRef.current value is set also fix this crash issue but root cause was unclear that's why going with other proposal

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @blimpich, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot]

📣 @badeggg 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[badeggg]

I have reported bug to react-navigation

[jayeshmangwani]

Thanks for the issue raising upstream

[badeggg]

Discussing the upstream bug here, seems I have to provide a minimal exact reproducible example to be convincible enough. I am working on that.

[badeggg]

I have made pull request.

It's an edge case and kind difficult to trigger if (isFocused !== valueToReturn), I'm still trying to provide a minimal exact reproducible example to upstream issue. Based on the evidences we have, I think we are confident enough to fix this issue.

I didn't patch the @react-navigation/core package, since I met this error You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file after I apply changes in this comment and npm run web, that seems requiring us amend babel config, which is not worthy.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.82-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• try fix 42916 loop crash #43338

If no regressions arise, payment will be issued on 2024-06-20. 🎊

For reference, here are some details about the assignees on this issue:

• @badeggg requires payment automatic offer (Contributor)
• @jayeshmangwani requires payment through NewDot Manual Requests

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@jayeshmangwani] The PR that introduced the bug has been identified. Link to the PR:
• [@jayeshmangwani] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@jayeshmangwani] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@jayeshmangwani] Determine if we should create a regression test for this bug.
• [@jayeshmangwani] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@johncschuster] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[johncschuster]

Will be paying this out on the 20th if there are no regressions. @jayeshmangwani, can you complete the BZ Checklist above before then? Thank you!

[jayeshmangwani]

• [@jayeshmangwani] The PR that introduced the bug has been identified. Link to the PR: fix: 24473 Deepliink - Keyboard displayed upon splash screen #30345
• [@jayeshmangwani] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: fix: 24473 Deepliink - Keyboard displayed upon splash screen #30345 (comment)
• [@jayeshmangwani] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
• [@jayeshmangwani] Determine if we should create a regression test for this bug.
• [@jayeshmangwani] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

1. Open the app and create a workspace if you do not already have one.
2. Tap the FAB, then go to Start Chat, and select #room.
3. If a workspace is not preselected, select a workspace.
4. After the keyboard opens, Press the back button from header.
5. Verify that the "Uh-oh, something went wrong" screen is not displayed and that the user is navigated to the LHN.

Do we agree 👍 or 👎

[johncschuster]

Payment Summary:

Contributor: @badeggg - $250 - paid via Upwork
Contributor+: @jayeshmangwani - $250 - paid via Manual request

[jayeshmangwani]

Requested on NewDot

[JmillsExpensify]

$250 approved for @jayeshmangwani