[HOLD for payment 2024-05-08] [$500] Public room - Login page shows momentarily before showing public room

[izarutskaya]

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

---

Version Number: 1.4.51-3
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4421788
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

precondition: user created a public room and copied the link

1. Log out of ND if logged in
2. Navigate to a public room via deeplink, e.g. https://staging.new.expensify.com/r/1275927936266854

Expected Result:

The user is navigated to a public room, and the chat history is displayed

Actual Result:

The user is navigated to the sign-in page. Only after login, the public room is opened

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

Bug6411899_1710306115660.video_2024-03-12_21-42-26.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0177518535cd3da4f9
• Upwork Job ID: 1770548324502310912
• Last Price Increase: 2024-03-27
• Automatic offers:
• tienifr | Contributor | 0

Issue Owner

Current Issue Owner: @puneetlath

[melvin-bot]

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[izarutskaya]

@puneetlath I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

[izarutskaya]

We think that this bug might be related to #vip-vsb
CC @quinthar

[bernhardoj]

This is a BE issue where some onyx data is in array.

[c3024]

PR #38272 of issue #37999 is awaiting fixing of this issue.

[mkhutornyi]

Can we fix this as priority? Looks like 🔥.

[melvin-bot]

@puneetlath Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

@puneetlath Still overdue 6 days?! Let's take care of this!

[puneetlath]

Hmm, so when I try it shows the login page momentarily but then does show the public room after a second.

Screen.Recording.2024-03-20.at.4.26.51.PM.mov

So @bernhardoj I feel like maybe something changed and this is indeed a client-side issue. What do you think?

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0177518535cd3da4f9

[melvin-bot]

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

[tienifr]

Proposal

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

The user is navigated to the sign-in page. Only after login, the public room is opened

What is the root cause of that problem?

In here, we're setting offlineStatus as true if state.isInternetReachable is null (the network status is unknown). When we first open the site, the network status will be unknown for a bit while the app is figuring out the real network status.

But we consider that as offline, so in here, we see that and show the sign in screen. The "assuming we're offline when network status is not determined" was on purpose to prevent performance issues where we try to send redundant Network calls to reconnect when the user was offline, we cannot just remove this logic as the below proposal suggested, it will reintroduce the performance issues.

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

We need to incorporate 'unknown' as a potential network status in the app, and only shows the sign in screen here if the network is explicitly offline. There're many ways to do this, here's an example:

1. Add a new key networkStatus to ONYXKEYS.NETWORK, which has 3 possible values: online, offline, unknown
2. When we set the isOffline status like here, we set the networkStatus correctly based on the state.isInternetReachable, if it's null, the networkStatus should be unknown
3. In places that should check the network is explicitly offline like here, we check networkStatus === 'offline' instead of checking the isNetworkOffline (which will be true even though the network status is unknown)

I think a new field like above should be the most intuitive and clean, if instead we want to reuse the isOffline field in ONYXKEYS.NETWORK , we can allow it to have 3 values: true, false and undefined/null (if it's undefined/null, it means the network status is unknown), but this will not be very clean and will be hard to manage. Or alternatively can add another boolean key isNetworkStatusDetermined, and use it along with the existing isOffline.

What alternative solutions did you explore? (Optional)

Similar to 3 above, there're certain places that require us to check that the network is explicitly online like here or here, which should use networkStatus === 'online' check. If we do this, we can make isOffline be false by default if the networkStatus is unknown (if we haven't confirmed that the user is offline, we assume they're online), this sounds more correct to me.

Besides, we might want to refactor existing places that use isOffline to use networkStatus === 'offline', and remove the isOffline field.

[kmbcook]

Proposal

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

Public room - Login page shows momentarily before showing public room

What is the root cause of that problem?

Onyx network.isOffline is temporarily set to true, causing the splash screen to be hidden too early.

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

Prevent setOfflineStatus from being called when state.isInternetReachable equals null, in both places shown below.

App/src/libs/NetworkConnection.ts

Line 109 in 729d475

setOfflineStatus((state.isInternetReachable ?? false) === false);

App/src/libs/NetworkConnection.ts

Line 65 in 6571b37

NetInfo.fetch().then((state) => setOfflineStatus((state.isInternetReachable ?? false) === false));

Because there are two places where this same change will be made, it would probably be a good idea to create a new function for this purpose.

const setOfflineStatusFromState = (state) => {
    if (state.isInternetReachable != null) {
        setOfflineStatus(!state.isInternetReachable);
    }
}

There is no condition in which it is appropriate to call setOfflineStatus in response to state.isInternetReachable being set to null.

[getusha]

@kmbcook tested your solution and i am still able to reproduce the issue.

[tienifr]

@allroundexperts PR #39595 is ready to review

[melvin-bot]

This issue has not been updated in over 15 days. @puneetlath, @allroundexperts, @tienifr eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[puneetlath]

@tienifr can you raise a fix for the regression above?

[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.68-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix login page appear for a while #39595

If no regressions arise, payment will be issued on 2024-05-08. 🎊

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

• @allroundexperts requires payment through NewDot Manual Requests
• @tienifr requires payment automatic offer (Contributor)

[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:

• [@allroundexperts] The PR that introduced the bug has been identified. Link to the PR:
• [@allroundexperts] 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:
• [@allroundexperts] 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:
• [@allroundexperts] Determine if we should create a regression test for this bug.
• [@allroundexperts] 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.
• [@puneetlath] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[puneetlath]

@tienifr has been paid.

@allroundexperts bump on the checklist for you.

[allroundexperts]

Checklist

1. I was not able to find the exact PR which caused this. It seems like several PRs introduced this issue with time.
2. N/A
3. N/A
4. A regression test would be helpful.

Regression Test

1. Log out of ND if logged in
2. Navigate to a public room via deeplink, e.g. https://staging.new.expensify.com/r/1275927936266854

Verify that the user is navigated to a public room, and the chat history is displayed

Do we 👍 or 👎 ?

[puneetlath]

Issue for regression test is here: https://github.com/Expensify/Expensify/issues/394854

Payment summary:

• C - $500 - @tienifr - paid via Upwork
• C+ - $500 - @allroundexperts - to be paid via NewDot

@allroundexperts please request on NewDot. Thanks everyone!

[JmillsExpensify]

$500 approved for @allroundexperts