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

[HOLD for payment 2022-11-24] [$500] mWeb- Blank page is displayed when tapped on Concierge or Send a message options #11892

Closed
kbecciv opened this issue Oct 16, 2022 · 45 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Oct 16, 2022

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


Issue found when executing PR #11857

Action Performed:

  1. Open the site https://help.expensify.com/main
  2. Click on Concierge button and verify it opens the Concierge chat in NewDot if the user is logged in.
  3. Click on the Send message button and verify it opens the Concierge chat in NewDot too.

Expected Result:

By clicking on the Send message button and verify it opens the Concierge chat in NewDot too.

Actual Result:

Blank page is displayed when tapped on Concierge or Send a message options

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Mobile Web

Version Number: 1.2.16.2

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

11857.mWeb1.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Oct 16, 2022

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

@flodnv
Copy link
Contributor

flodnv commented Oct 17, 2022

@kbecciv can you confirm this only happens on Mobile Web? I have tested on Desktop and also on Android with the native app and it works.

cc @marcochavezf as you are the author of #11857. Can this be external? 🤔

@kbecciv
Copy link
Author

kbecciv commented Oct 17, 2022

@flodnv
Correct, it happens only on mobile Web. Windows/Chrome is Pass for QA team.

11857.Web.mp4

@marcochavezf
Copy link
Contributor

Oh interesting it only happens in mWeb (no native app installed). Yeah it can be external. FWIW this issue is not related to the help.expensify.com site, the problem is that for some reason https://new.expensify.com/concierge is not opening the Concierge report correctly only on mWeb (only if the user is logged in).

@flodnv flodnv added the External Added to denote the issue can be worked on by a contributor label Oct 18, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 18, 2022

Triggered auto assignment to @lschurr (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Oct 18, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 18, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 18, 2022

Current assignee @flodnv is eligible for the External assigner, not assigning anyone new.

@melvin-bot melvin-bot bot changed the title mWeb- Blank page is displayed when tapped on Concierge or Send a message options [$250] mWeb- Blank page is displayed when tapped on Concierge or Send a message options Oct 18, 2022
@lschurr
Copy link
Contributor

lschurr commented Oct 18, 2022

@lschurr lschurr added Weekly KSv2 and removed Daily KSv2 labels Oct 18, 2022
@puneetlath puneetlath added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 19, 2022
@rushatgabhane
Copy link
Member

@lschurr time to double? We don't have any proposals yet

@lschurr
Copy link
Contributor

lschurr commented Oct 25, 2022

@lschurr lschurr changed the title [$250] mWeb- Blank page is displayed when tapped on Concierge or Send a message options [$500] mWeb- Blank page is displayed when tapped on Concierge or Send a message options Oct 25, 2022
@Ollyws
Copy link
Contributor

Ollyws commented Oct 29, 2022

Proposal

The root cause of this bug is due to the recent addition of react-freeze, specifically in SidebarLinks.
This causes the LHN to freeze on small screens if the drawer is closed, which (when navigating directly to a chat) means App.setSidebarLoaded in the onLayout is never called and ONYXKEYS.IS_SIDEBAR_LOADED is never set to true.

This causes a problem when navigating directly to a chat on mobile because ReportScreen will not render unless ONYXKEYS.IS_SIDEBAR_LOADED is true, leaving the user with a blank screen.

One solution to this is to subscribe to ONYXKEYS.IS_SIDEBAR_LOADED in SidebarLinks and only enable Freeze after it has been set to true, however this causes an unnecessary re-render.

A better solution is to assign a variable in onLayout after App.setSideBarLoaded and only enable Freeze when it is true:

<Freeze freeze={this.props.isSmallScreenWidth && !this.props.isDrawerOpen}>
<LHNOptionsList
contentContainerStyles={[
styles.sidebarListContainer,
{paddingBottom: StyleUtils.getSafeAreaMargins(this.props.insets).marginBottom},
]}
data={optionListItems}
focusedIndex={_.findIndex(optionListItems, (
option => option.toString() === this.props.reportIDFromRoute
))}
onSelectRow={(option) => {
Navigation.navigate(ROUTES.getReportRoute(option.reportID));
this.props.onLinkClick();
}}
shouldDisableFocusOptions={this.props.isSmallScreenWidth}
optionMode={this.props.priorityMode === CONST.PRIORITY_MODE.GSD ? 'compact' : 'default'}
onLayout={App.setSidebarLoaded}
/>
</Freeze>

-               <Freeze freeze={this.props.isSmallScreenWidth && !this.props.isDrawerOpen}>
+               <Freeze freeze={this.props.isSmallScreenWidth && !this.props.isDrawerOpen && this.isSidebarLoaded}>
                    <LHNOptionsList
                        contentContainerStyles={[
                            styles.sidebarListContainer,
                            {paddingBottom: StyleUtils.getSafeAreaMargins(this.props.insets).marginBottom},
                        ]}
                        data={optionListItems}
                        focusedIndex={_.findIndex(optionListItems, (
                            option => option.toString() === this.props.reportIDFromRoute
                        ))}
                        onSelectRow={(option) => {
                            Navigation.navigate(ROUTES.getReportRoute(option.reportID));
                            this.props.onLinkClick();
                        }}
                        shouldDisableFocusOptions={this.props.isSmallScreenWidth}
                        optionMode={this.props.priorityMode === CONST.PRIORITY_MODE.GSD ? 'compact' : 'default'}
-                       onLayout={App.setSidebarLoaded}
+                       onLayout={() => {
+                           App.setSidebarLoaded();
+                           this.isSidebarLoaded = true;
+                       }}
                    />
                </Freeze>

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 29, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 29, 2022

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

@lschurr
Copy link
Contributor

lschurr commented Oct 31, 2022

Hi @rushatgabhane - could you review the added proposal?

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@melvin-bot melvin-bot bot added the Overdue label Nov 3, 2022
@Ollyws
Copy link
Contributor

Ollyws commented Nov 8, 2022

@lschurr Offer accepted.

@mvtglobally
Copy link

Issue not reproducible during KI retests. (Second week)

@Ollyws
Copy link
Contributor

Ollyws commented Nov 9, 2022

@lschurr @rushatgabhane Can I submit the PR now or do I need to wait until I'm assigned to this?

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 9, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 9, 2022

📣 @Ollyws You have been assigned to this job by @flodnv!
Please apply to this job in Upwork 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 📖

@flodnv
Copy link
Contributor

flodnv commented Nov 9, 2022

Go for it @Ollyws !

@Ollyws
Copy link
Contributor

Ollyws commented Nov 9, 2022

All yours @rushatgabhane

@flodnv
Copy link
Contributor

flodnv commented Nov 11, 2022

@rushatgabhane can you review the PR please?

@melvin-bot
Copy link

melvin-bot bot commented Nov 11, 2022

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:

  • @lschurr A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here: https://github.com/Expensify/Expensify/issues/242924
  • @rushatgabhane @flodnv The PR that introduced the bug has been identified. Link to the PR: Enable Modern Drawer in react-navigation #11550
  • @lschurr 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:
  • @lschurr A discussion in #contributor-plus 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:
  • @lschurr Payment has been made to the issue reporter (if applicable)
  • @lschurr Payment has been made to the contributor that fixed the issue (if applicable)
  • @lschurr Payment has been made to the contributor+ that helped on the issue (if applicable)

@melvin-bot melvin-bot bot added the Overdue label Nov 14, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 14, 2022

@flodnv, @Ollyws, @rushatgabhane, @lschurr Whoops! This issue is 2 days overdue. Let's get this updated quick!

@flodnv
Copy link
Contributor

flodnv commented Nov 14, 2022

@lschurr next steps are on you^

@melvin-bot melvin-bot bot removed the Overdue label Nov 14, 2022
@lschurr
Copy link
Contributor

lschurr commented Nov 14, 2022

Asked for help here since I'm unsure about which test should be added. https://expensify.slack.com/archives/C01SKUP7QR0/p1668439426176659

@flodnv
Copy link
Contributor

flodnv commented Nov 14, 2022

We should add these, from mWeb :
image

@lschurr
Copy link
Contributor

lschurr commented Nov 14, 2022

Ok, I made this GH to request the test be added. @flodnv would you look at that and confirm I've done it correctly? (sorry first time doing all of this)

@flodnv
Copy link
Contributor

flodnv commented Nov 15, 2022

LGTM, thanks 👍

@lschurr
Copy link
Contributor

lschurr commented Nov 15, 2022

@rushatgabhane - I've invited you to apply for the job in Upwork. Can you accept the offer?

@mvtglobally
Copy link

Issue not reproducible during KI retests. (Third week)

@rushatgabhane
Copy link
Member

@lschurr accepted, thanks!

@lschurr
Copy link
Contributor

lschurr commented Nov 16, 2022

@Ollyws and @rushatgabhane have been paid! Closing.

@lschurr lschurr closed this as completed Nov 16, 2022
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Nov 17, 2022
@melvin-bot melvin-bot bot changed the title [$500] mWeb- Blank page is displayed when tapped on Concierge or Send a message options [HOLD for payment 2022-11-24] [$500] mWeb- Blank page is displayed when tapped on Concierge or Send a message options Nov 17, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 17, 2022

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

If no regressions arise, payment will be issued on 2022-11-24. 🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants