[iOS] Global create is open on account creation/sign-in, but isn't visible #5303

JmillsExpensify · 2021-09-16T18:39:07Z

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

Create a new account
When you sign in, Global create is open but invisible
If you close Global Create and re-open, it shows up

Expected Result:

Global create should be open and visible on account creation

Actual Result:

Global create doesn't show up

Workaround:

Workaround is close and re-open, but this is a critical first-user experience, so this workaround isn't acceptable

Platform:

Where is this issue occurring?

iOS

Version Number: 1.0.99-1
Reproducible in staging?: Yes
Reproducible in production?: Yes
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL: N/A
Issue reported by: Marc
Slack conversation: https://expensify.slack.com/archives/C020EPP9B9A/p1631816718390200

View all open jobs on GitHub

MelvinBot · 2021-09-16T18:39:10Z

Triggered auto assignment to @jboniface (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

MelvinBot · 2021-09-16T18:42:01Z

Triggered auto assignment to @ctkochan22 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

ctkochan22 · 2021-09-17T02:14:39Z

Reproduced while testing fix for https://github.com/Expensify/Expensify/issues/177787#issuecomment-921377044

Reproduction step:

User must be new. NVP "isFirstTimeNewExpensifyUser" must be true. This will cause the create menu to open automatically
You can guarantee it by setting the time out to 0 here:

App/src/pages/home/sidebar/SidebarScreen.js

Lines 84 to 86 in 09e2d94

setTimeout(() => {

this.toggleCreateMenu();

}, 200);

ctkochan22 · 2021-09-17T04:50:48Z

I bet it has something to do with why we need a timeout on this line:

App/src/pages/home/sidebar/SidebarScreen.js

Lines 84 to 86 in 09e2d94

    
           setTimeout(() => { 
        
               this.toggleCreateMenu(); 
        
           }, 200);

By setting timeout to 0, and NVP "isFirstTimeNewExpensifyUser" to true, i can reproduce this consistently. I think this means, we are opening it before its either gotten the info it needs, or before something can be built/rendered?

ctkochan22 · 2021-09-17T05:57:20Z

@Beamanator do you have an idea? I see you are familiar with this area

Beamanator · 2021-09-17T10:27:10Z

I'm the one who merged in this setTimeout since I couldn't find a better solution 😅

I created another issue to get contributor ideas why setTimeout was needed and how to replace it: #5296

I'll try Marc's idea here: #5214 (comment)

Beamanator · 2021-09-17T11:51:46Z

Here's what happened when I tried @marcaaron 's idea, and the changes I made:

Workflow:

Open app on iOS, iPhone 12, version 14.5
Sign up with brand new account
Validate account using CLI
Log in with new account
Global create didn't open (no X either)

Code changed & logs added:

All code changes recommended here: Address #5047 - Open global create when a user logs in for the first time #5214 (comment)
NameValuePair.get(): (added a log)

function get(name, onyxKey, defaultValue) {
    API.Get({
        returnValueList: 'nameValuePairs',
        name,
    })
        .then((response) => {
            const value = lodashGet(response.nameValuePairs, [name], defaultValue || '');
            // Log the time this is called so we know when Onyx.set "should have been" called
            if (name === 'isFirstTimeNewExpensifyUser') console.log('[AuthScreens] setting nvp to ', value);
            Onyx.set(onyxKey, value);
        });
}

SidebarScreen.js:
- In componentDidMount, add log: console.log('[SidebarScreen] componentDidMount');
- In componentDidUpdate, add log: console.log([SidebarScreen] componentDidUpdate (prev:${prevProps.isFirstTimeNewExpensifyUser})(now:${this.props.isFirstTimeNewExpensifyUser}));
In react-native-onyx/lib/Onyx.js:
- In function set, add log at the beginning: if (key === 'isFirstTimeNewExpensifyUser') console.log('[Onyx.set]', key, val);

Order of logs when following the workflow:

 LOG  [AuthScreens] setting nvp to  true
 LOG  [Onyx.set] isFirstTimeNewExpensifyUser true
 LOG  [SidebarScreen] componentDidMount
 LOG  [SidebarScreen] componentDidUpdate (prev:false)(now:false)
 LOG  [SidebarScreen] componentDidUpdate (prev:false)(now:false)
 ... repeated 7 more times

Question: In SidebarScreen's componentDidUpdate, why is this.props.isFirstTimeNewExpensifyUser never true?? The log in Onyx.set shows it does get called with true so it should be updating and re-triggering componentDidMount with the value true!!

cc @kidroca since you're pretty experienced with react-native-onyx

kidroca · 2021-09-17T13:31:33Z

@Beamanator
You get false in componentDidUpdate because componentDidMount sets it to false

if (this.props.isFirstTimeNewExpensifyUser) {
  /* Bunch of code */

  // Set the NVP to false so we don't automatically open the menu again
  // Note: this may need to be moved if this NVP is used for anything else later
  NameValuePair.set(CONST.NVP.IS_FIRST_TIME_NEW_EXPENSIFY_USER, false, ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER);
}

So it's initially true during componentDidMount and entered the above flow

I've deliberately altered this to force a true condition

src/libs/actions/NameValuePair.js

function get(name, onyxKey, defaultValue) {
    API.Get({
        returnValueList: 'nameValuePairs',
        name,
    })
        .then((response) => {
            let value = lodashGet(response.nameValuePairs, [name], defaultValue || '');

            if (onyxKey === 'isFirstTimeNewExpensifyUser') {
                console.log('isFirstTimeNewExpensifyUser: ', value);
                value = true;
            }

            Onyx.set(onyxKey, value);
        });
}

Global create didn't open (no X either)

It's seems it might have to do with the other expression const hasFreePolicy = _.chain(this.props.policies) or however the code was changed. Maybe you can share push your local changes?

Beamanator · 2021-09-17T14:03:14Z

@kidroca yeah sorry my notes may not have been super clear, I ended up stripping most of the stuff in componentDidMount and moving it to componentDidUpdate, as suggested as a possible solution here

Here's a branch with what I was testing: https://github.com/Expensify/App/tree/beaman-investigatingGlobalCreateFishyness

In this branch you can see I'm not doing const hasFreePolicy = _.chain(this.props.policies) anymore (just for testing - we'd want to add it back so global create doesn't open if the user has a workspace already)

Note: As mentioned above, I also added a log in In react-native-onyx/lib/Onyx.js's set function:

Add log at the beginning: if (key === 'isFirstTimeNewExpensifyUser') console.log('[Onyx.set]', key, val);

kidroca · 2021-09-17T14:30:28Z

What I've found out is that without the setTimeout the modal won't appear (ios)(you will see the dismiss (x) though), a similar issue we had with the attachment picker
It seems to be a known issue with iOS and Modals, but when I tried to recreated it in a standalone expo snack it didn't happen and the modal always appeared (I'm talking about the issue with the attachment picker).
So this might be something that we're doing as we have multiple component layers and HOCs until we render the Modal

kidroca · 2021-09-17T15:18:48Z

@Beamanator
The code changes work for me in that they raise the true flag in the componentDidUpdate method
But due to the lack of setTimeout I only see the (x) button - the modal is invisible

The reason you don't get the value in componentDidUpdate is because that's not the only Onyx connection that this component is using - the component won't mount until the keys betas, policies are read from storage as they don't have the initWithStoredValues: false setting.
And my theory is this happens:

Backend responds before this component has mounted and sets the value to true. Now that's the value in storage
This component hasn't mounted because not all Onyx keys are ready
When the component mounts it does not use the value stored in Onyx - due to initWithStoredValues: false
When componentDidUpdate runs you see the defaultProps.isFirstTimeNewExpensifyUser which is false

cc @marcaaron @tgolen I think this is a caveat about initWithStoredValues: false:
When a component uses other Onyx keys that don't have this flag - see the explanation above ⬆️
Basically storage gets updated before the component has mounted, and so we miss the update that were waiting for by using initWithStoredValues: false

kidroca · 2021-09-17T15:31:48Z

Here's a non setTimeout based solution, though it's against the Expensify, "no Promise" policy, but I think it shows a use case where it's favorable to have the option to use Promises

Make NameValuePair methods async

/* we want to open the menu for first time experience, and then we flag the experience as complete */
NameValuePair
    .get(CONST.NVP.IS_FIRST_TIME_NEW_EXPENSIFY_USER, ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER, true)
    .then(value => this.setState({isCreateMenuActive: value}))
    .then(() => NameValuePair.set(CONST.NVP.IS_FIRST_TIME_NEW_EXPENSIFY_USER, false, ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER));

Here's a full diff:
main...kidroca:kidroca/global-create-fishyness
This way handling is much simple and runs only once. You don't have to setup a check that runs for every componentDidMount

Screen.Recording.2021-09-17.at.18.07.20.mov

kidroca · 2021-09-17T15:37:37Z

To prove there's some weird bug with the Modal on iOS you can force the Modal to be open right from the start

src/pages/home/sidebar/SidebarScreen.js

        this.state = {
            isCreateMenuActive: true,
        };

Refresh and you'll end up with this:

ctkochan22 · 2021-09-17T15:51:03Z

Ooo interesting. Let me test this out locally (sorry just catching up)

ctkochan22 · 2021-09-17T17:17:08Z

Still trying to grasp the issue. If i'm following your logic correctly, @kidroca , you're saying it doesn't run componentDidMount() until the ONYX keys are loaded. So it defaults to defaultProps.isFirstTimeNewExpensifyUser which is not set yet?

marcaaron · 2021-09-17T17:22:37Z

I think Peter is saying this isn't a React lifecycle problem, but maybe a bug with react-native-modal on iOS? i.e. it still won't open even with a hardcoded value of isVisible={true}.

kidroca · 2021-09-17T17:33:21Z

If i'm following your logic correctly, @kidroca , you're saying it doesn't run componentDidMount() until the ONYX keys are loaded.

Correct.
This is only a problem with the code that sets initWithStoredValues: false and moves the logic to componentDidUpdate

So it defaults to defaultProps.isFirstTimeNewExpensifyUser which is not set yet?

That prop is not getting any updates after the component mounts
Other prop changes are triggering componentDidUpdate and it prints that isFirstTimeNewExpensifyUser was false and remained false
We were waiting for it to be true to trigger toggleMenu but that never happened
At least that's what happens for Alex - I was able to receive an updated value in componentDidUpdate, but it depends - when the key is set before the component mounts we'll miss it

To make that work correctly we'll want to remove initWithStoredValues: false and handle the isFirstTimeNewExpensifyUser in both didMount and didUpdate, and even then we'll probably still have the problem

When you sign in, Global create is open but invisible

Judging by the problem description, the "existing" logic is working correctly, it's just that it "fires" too soon (even with a timeout)
as when you hardcode the modal to open: #5303 (comment)

parasharrajat · 2021-09-17T17:43:08Z

I think Peter is saying this isn't a React lifecycle problem, but maybe a bug with react-native-modal on iOS? i.e. it still won't open even with a hardcoded value of isVisible={true}.

I don't think it's true. https://snack.expo.dev/@parasaharrajat/react-native-modal-example

Check the Snack. It works great.

kidroca · 2021-09-17T17:43:26Z

I think Peter is saying this isn't a React lifecycle problem, but maybe a bug with react-native-modal on iOS? i.e. it still won't open even with a hardcoded value of isVisible={true}.

Yes, I've seen others have the problem opening a modal as soon as another modal closes, which results in a similar experience - isPopoupVisible would be true while there would be no modal on screen
But I couldn't recreate it in a standalone environment to submit a bug for react-native-modal so it might be on us

parasharrajat · 2021-09-20T12:15:43Z

I made sure to test this scene, but react-native-modal instance is not receiving isVisible:true for the FAbMenu modal on load. Yeah, baseModal is correctly seeing true for isVisible prop.
Then I directly passed true to react-native-modal instance in BaseModal but isVisible was not passed true for a single render.

I tried reducing the render to minimal but I found that it's not affected by rerendering.

roryabraham · 2021-09-20T16:37:29Z

I saw the isVisible prop being passed correctly to the react-native-modal from the SidebarScreen -> PopoverMenu -> BasePopoverMenu -> ReactNativeModal. So I think @kidroca is correct that isVisible is being passed correctly and that there's some race condition at play with the drawerContent.

roryabraham · 2021-09-20T16:39:11Z

Tried simply wrapping the SidebarScreen with DrawerContentScrollView as suggested by the docs here but that didn't solve the problem.

parasharrajat · 2021-09-20T17:01:51Z

I ran a debugger over the react-native-modal and the logic inside never ran where we are checking props.isvisible is true in the onLoad scene.

I removed the nested props and directly set the isVisible true in BaseModal and the result was the same.

I am not sure what could be the reason that React is not passing that prop correctly.

I think the solution here by Kidroca is correct. We should wait for the drawer to be shown completely and then show the modal if that works.

roryabraham · 2021-09-20T17:11:12Z

The drawerContent receives a progress prop: reactnavigation.org/docs/drawer-navigator#drawercontent
Maybe we have to make sure the progress has settled before opening the Modal

Was trying this, but ran into an issue where this prop just isn't present: react-navigation/react-navigation#9878

roryabraham · 2021-09-20T17:41:04Z

In the meantime, can we maybe fix this more consistently by just increasing the timeout to 1500 ms or something? AFAICT that wouldn't result in a very adverse effect in the UX – when you open the app for the first time, 1.5 seconds later the global create menu opens. This is prettymuch the effect we want, and the minor delay likely isn't going to have an effect on usability. This one-time action is not a performance-critical flow, so the hack might be acceptable for now. Thoughts?

kidroca · 2021-09-20T17:59:36Z

In the meantime, can we maybe fix this more consistently by just increasing the timeout to 1500 ms or something?

I think if we're in a hurry and it wont hurt UX, it'll be ok for the moment
I've tried different things, but nothing helped, like:

removing mode="modal" from AuthScreens
adding detachInactiveScreens={false} to BaseDrawerNavigator

We should at least change the logic for the check to be inside the setInterval e.g.

    componentDidMount() {
        Performance.markStart(CONST.TIMING.SIDEBAR_LOADED);
        Timing.start(CONST.TIMING.SIDEBAR_LOADED, true);

        setTimeout(() => {
            if (this.props.isFirstTimeNewExpensifyUser) {
                 /* rest of the code */
            }
        }, 1500);
    }

This way we're using the latest isFirstTimeNewExpensifyUser value, and not the one from "mount time" which might not have been set by the response

parasharrajat · 2021-09-20T18:00:11Z

Yeah, an average delay should be added as it does not feel good when using the first time, active modal was shown instead it should animate before the user with an instruction that what it does.

Instruction could be added in the blank area(grayed part) over the menu.

Jag96 · 2021-09-20T20:30:05Z

I think if we're in a hurry and it wont hurt UX, it'll be ok for the moment

Agreed, this seems like the easiest fix right now

We should at least change the logic for the check to be inside the setInterval

+1 as well

Yeah, an average delay should be added as it does not feel good when using the first time, active modal was shown instead it should animate before the user with an instruction that what it does.

I agree adding some onboarding indications could make the UX better here, but I think the 1500ms will work for now.

roryabraham · 2021-09-20T21:10:02Z

Noticing a lot of people are assigned to this issue, but I'm going to submit a pull request to increase the timeout. Please DM me if you're already working on this.

MitchExpensify · 2021-09-21T00:17:31Z

No closed Botify

roryabraham · 2021-09-21T00:43:44Z

Why no closed?

MitchExpensify · 2021-09-21T01:12:12Z

I thought we closed when the PR hit production

roryabraham · 2021-09-21T04:23:53Z

For external contributors we close the issue 7 days after the fix has been deployed to production. For internal contributors we generally close it right away (and reopen if the PR fails QA)

marcaaron · 2022-04-28T21:06:40Z

I'm trying to understand this issue a bit to clear up the setTimeout() but not able to reproduce the bug anymore.

I can see it happen if we try to initialize the global create menu with isActive: true, but if we set to state after the component mounts then everything works fine.

Maybe it only affects certain versions of iOS...

marcaaron · 2022-04-28T22:35:25Z

Tested on iOS 13-15 and can't get it to happen. I think the moving the logic in this PR fixed the problem, but unsure why.

luacmartins · 2022-04-28T23:27:49Z

It was definitely still happening then, because I tried to remove it and failed. Although I thought I left a comment somewhere about it fixing a race condition and not a bug with react-navigation.

kidroca · 2022-04-29T08:37:53Z

I think it depends on render/networking time

Looks like the bug is between our code, react native navigation and react native modal, but it's not clear which side the setTimeout fixes

Initializing isActive: true in Drawer/Sidebar does not show the modal
Moving the component out of the Sidebar and initializing with isActive: true did work
Initializing with isActive: false and then changing it - sometimes work

I suppose it's a problem with the View Controller react native modal tries to use to manage modals and the navigation initializing

Similar problem that has no development but using setTimeout is the modal to modal transition (e.g. (+) -> attachment -> camera)
Though the problem there is that without setTimeout closing the modal happens at the same time as camera/gallery appearing and it somehow closes the camera as well

marcaaron · 2022-04-29T16:32:47Z

Initializing with isActive: false and then changing it - sometimes work

Interesting, I'm trying to find out what the "sometimes" factor is here. So far, it has been working fine for me and anyone else who has tested on iOS so I'm inclined to see if it will pop up again.

kidroca · 2022-04-29T16:45:18Z

Initializing with isActive: false and then changing it - sometimes work

Interesting, I'm trying to find out what the "sometimes" factor is here. So far, it has been working fine for me and anyone else who has tested on iOS so I'm inclined to see if it will pop up again.

As far as I remember it depended on the timing of the response to one "get keyval pair"
Maybe the logic changed and it's no longer valid
Mind if I take a look? (Is there a branch about this?)

marcaaron · 2022-04-29T17:32:58Z

Changes here are what I have been testing -> #8827

JmillsExpensify added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Sep 16, 2021

MelvinBot assigned jboniface Sep 16, 2021

MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Sep 16, 2021

marcaaron added the Engineering label Sep 16, 2021

jboniface removed their assignment Sep 16, 2021

jboniface added Engineering and removed Engineering labels Sep 16, 2021

MelvinBot assigned ctkochan22 Sep 16, 2021

Beamanator changed the title ~~Global create is open on account creation/sign-in, but isn't visible~~ [iOS] Global create is open on account creation/sign-in, but isn't visible Sep 17, 2021

Beamanator self-assigned this Sep 17, 2021

luacmartins self-assigned this Sep 20, 2021

roryabraham mentioned this issue Sep 20, 2021

[HOLD] [iOS] Figure out why setTimeout is needed when viewing SidebarScreen on load, replace with better solution #5296

Closed

roryabraham mentioned this issue Sep 20, 2021

Increase global create modal delay to 1500ms #5369

Merged

5 tasks

botify closed this as completed Sep 20, 2021

MitchExpensify reopened this Sep 21, 2021

thesahindia mentioned this issue Sep 21, 2021

[HOLD for payment 2021-10-07] Use number pad keyboard when field input will only include numbers in VBA flow #5321

Closed

Beamanator mentioned this issue Sep 21, 2021

Skip showing create for new users if user already has a workspace #5244

Merged

2 tasks

MitchExpensify closed this as completed Sep 21, 2021

kidroca mentioned this issue Oct 14, 2021

Remove setTimeout usage in AttachmentPicker/index.native.js #2719

Closed

[iOS] Global create is open on account creation/sign-in, but isn't visible #5303

[iOS] Global create is open on account creation/sign-in, but isn't visible #5303

Comments

JmillsExpensify commented Sep 16, 2021

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platform:

MelvinBot commented Sep 16, 2021

MelvinBot commented Sep 16, 2021

ctkochan22 commented Sep 17, 2021 • edited Loading

ctkochan22 commented Sep 17, 2021 • edited Loading

ctkochan22 commented Sep 17, 2021

Beamanator commented Sep 17, 2021

Beamanator commented Sep 17, 2021 • edited Loading

kidroca commented Sep 17, 2021

src/libs/actions/NameValuePair.js

Beamanator commented Sep 17, 2021 • edited Loading

kidroca commented Sep 17, 2021 • edited Loading

kidroca commented Sep 17, 2021 • edited Loading

kidroca commented Sep 17, 2021

kidroca commented Sep 17, 2021

src/pages/home/sidebar/SidebarScreen.js

ctkochan22 commented Sep 17, 2021

ctkochan22 commented Sep 17, 2021

marcaaron commented Sep 17, 2021

kidroca commented Sep 17, 2021 • edited Loading

parasharrajat commented Sep 17, 2021

kidroca commented Sep 17, 2021

parasharrajat commented Sep 20, 2021

roryabraham commented Sep 20, 2021

roryabraham commented Sep 20, 2021

parasharrajat commented Sep 20, 2021 • edited Loading

roryabraham commented Sep 20, 2021

roryabraham commented Sep 20, 2021 • edited Loading

kidroca commented Sep 20, 2021 • edited Loading

parasharrajat commented Sep 20, 2021 • edited Loading

Jag96 commented Sep 20, 2021

roryabraham commented Sep 20, 2021

MitchExpensify commented Sep 21, 2021

roryabraham commented Sep 21, 2021

MitchExpensify commented Sep 21, 2021

roryabraham commented Sep 21, 2021

marcaaron commented Apr 28, 2022

marcaaron commented Apr 28, 2022

luacmartins commented Apr 28, 2022

kidroca commented Apr 29, 2022

marcaaron commented Apr 29, 2022

kidroca commented Apr 29, 2022

marcaaron commented Apr 29, 2022

ctkochan22 commented Sep 17, 2021 •

edited

Loading

ctkochan22 commented Sep 17, 2021 •

edited

Loading

Beamanator commented Sep 17, 2021 •

edited

Loading

Beamanator commented Sep 17, 2021 •

edited

Loading

kidroca commented Sep 17, 2021 •

edited

Loading

kidroca commented Sep 17, 2021 •

edited

Loading

kidroca commented Sep 17, 2021 •

edited

Loading

parasharrajat commented Sep 20, 2021 •

edited

Loading

roryabraham commented Sep 20, 2021 •

edited

Loading

kidroca commented Sep 20, 2021 •

edited

Loading

parasharrajat commented Sep 20, 2021 •

edited

Loading