fix/14490: No data displayed in options list if component using report data is rendered too early #17349

jczekalski · 2023-04-12T16:49:46Z

Details

The issue affects components that rely on report and personal details data to display a list of options. If the component is rendered too early, the data will not be displayed even when it has fully loaded or, if personal details is not ready, the app will crash. This issue was originally identified on SearchPage, where, if the page is opened too early, the data would only be displayed after closing and re-opening the page or typing in the input field.

Pages/components affected by this problem:

SearchPage
NewChatPage / NewGroupPage
MoneyRequestParticipantsSelector
MoneyRequestParticipantsSplitSelector
WorkspaceInvitePage

The purpose of this PR is to:

make sure the UI updates once data is available / when data changes
display a skeleton loading state while data is loading
prevent a crash that could occur when trying to create list options before personal details data ready
fix crash when WorkspaceMembersPage is opened via deep link after logging out

Fixed Issues

$ #14490
$ #17293
$ #16739
$ #18389
PROPOSAL: #14490 (comment)

Tests

Open the app
Set network throttling to Slow 3G or slower
Open one of the pages affected by the problem as soon as the app loads

Search Page (Cmd + K)
New Chat Page / New Group Page
Send Money / Request Money / Split Bill -> enter amount -> tap Next

The options list should display a skeleton loading state
Data should replace the loading state once available
If search term is entered while data is loading, it should be applied once data is displayed

Verify that no errors appear in the JS console

Offline tests

Web

To make it easier to reproduce this issue, set network throttling to Slow 3G or add an even slower profile, e.g.

Mac

Simulate a bad network using Network Link Conditioner

QA Steps

Open the app
Set network throttling to Slow 3G or slower
Open one of the pages affected by the problem as soon as the app loads

Search Page (Cmd + K)
New Chat Page / New Group Page
Send Money / Request Money / Split Bill -> enter amount -> tap Next

The options list should display a skeleton loading state
Data should replace the loading state once available
If search term is entered while data is loading, it should be applied once data is displayed

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Skeleton loader - Search Page (no search term)

demo_search_page.mov

Skeleton loader - New Chat (with search term entered while loading)

demo_new_chat_search_term.mov

Skeleton loader - Send Money

demo_send_money.mov

Report and personal details data updating

chat_personal_details_udodates.mov

Mobile Web - Chrome

android_chrome.mov

Mobile Web - Safari

ios_safari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

MelvinBot · 2023-04-12T16:49:52Z

@amyevans @aimane-chnaif One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

github-actions · 2023-04-12T16:50:01Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

jczekalski · 2023-04-12T16:53:44Z

I have read the CLA Document and I hereby sign the CLA

jczekalski · 2023-04-12T17:19:04Z

recheck

aimane-chnaif · 2023-04-12T17:21:50Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@jczekalski no worries. as bot says, you signed already.

jczekalski · 2023-04-13T10:55:41Z

PR is ready for review @aimane-chnaif @amyevans

…ptions list

aimane-chnaif · 2023-04-14T11:11:04Z

@jczekalski don't use rebase merge. too many commits and irrelevant code changes. can you repair PR?

jczekalski · 2023-04-14T11:14:20Z

@jczekalski don't use rebase merge. too many commits and irrelevant code changes. can you repair PR?

@aimane-chnaif Yes, sorry, I tried to update my branch with main since some conflicts showed up and I messed it up.

jczekalski · 2023-04-14T13:54:34Z

@aimane-chnaif Fixed

aimane-chnaif · 2023-04-18T13:14:16Z

@jczekalski please pull from main

jczekalski · 2023-04-19T10:24:31Z

@jczekalski please pull from main

@aimane-chnaif done

amyevans · 2023-04-19T13:36:04Z

@aimane-chnaif will you be able to review/test today?

aimane-chnaif · 2023-04-19T13:38:23Z

@aimane-chnaif will you be able to review/test today?

yes

aimane-chnaif · 2023-04-20T04:26:07Z

@jczekalski Did you push all the changes? I don't think PR fixes main bugs happening on search, money request pages.
Showing skeleton forever, app crashes very often

bug1.mov

bug2.mov

bug3.mov

aimane-chnaif · 2023-04-20T04:26:20Z

Also please pull from main and fix conflicts

aimane-chnaif · 2023-04-20T04:39:41Z

Also let's make sure that our PR fixes #17293, #16739 which were closed in favor of our issue.
So please follow their reproducible steps and fix them. The root cause and solution is well explained here.

jczekalski · 2023-04-20T08:01:09Z

@jczekalski Did you push all the changes? I don't think PR fixes main bugs happening on search, money request pages. Showing skeleton forever, app crashes very often

bug1.mov
bug2.mov
bug3.mov

@aimane-chnaif You're right, it's not working for me either. Something must have changed since I originally opened the PR, I'll try to figure out what's going on.

jczekalski · 2023-04-20T08:04:31Z

Also let's make sure that our PR fixes #17293, #16739 which were closed in favor of our issue. So please follow their reproducible steps and fix them. The root cause and solution is well explained here.

I'm not familiar with those issues, I'll take a look.

jczekalski · 2023-04-20T10:41:38Z

@aimane-chnaif I pulled in the changes from the latest version of main and everything is working correctly again.

amyevans · 2023-05-05T13:58:20Z

Thanks guys, I agree there's been some scope creep here and sympathize that that can be frustrating. Can you quickly summarize how you'd like any out-of-scope work split up into new GH(s) and I can get those created?

I can prioritize a final PR review by the end of my day as well so that we can hopefully get this merged.

jczekalski · 2023-05-05T15:12:58Z

@aimane-chnaif @amyevans
I haven't faced any other issues, but these issues are pretty tricky to find since you have to open a screen before the Onyx data is ready or just deep link directly into it while logged out. I think we need a new issue for:

remove code that prevents the Settings screen from rendering before currentUserPersonalDetails is ready

 render() {
        // On the very first sign in or after clearing storage these
        // details will not be present on the first render so we'll just
        // return nothing for now.
        if (_.isEmpty(this.props.currentUserPersonalDetails)) {
            return null;
        }

nested screens can now be navigated to earlier than before
make sure there are no issues by testing with a slow connection or deep linking into the nested screens
we might also need some new loading states? e.g. after removing this code the text below the avatar on the Settings screen flickers from email address to name + surname

aimane-chnaif · 2023-05-08T06:26:51Z

There are still issues on some screens but as agreed we can fix them as separate issues.
All of them can be easily tested on desktop app opened from web deep link (after logout but without re-login).

Follow-up issues:

crash on split confirm (example link: http://localhost:8080/split/new/397893873612112)

split.confirm.crash.mov

users list not showing on split confirm (example link: http://localhost:8080/split/new/397893873612112)

split.confirm.list.mov

not found view briefly appears on workspace members, invite pages

invite.mov

members.mov

aimane-chnaif · 2023-05-08T06:32:06Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

mchrome.mov

Mobile Web - Safari

msafari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

jczekalski · 2023-05-08T09:57:43Z

There are still issues on some screens but as agreed we can fix them as separate issues. All of them can be easily tested on desktop app opened from web deep link (after logout but without re-login).

Follow-up issues:

crash on split confirm (example link: http://localhost:8080/split/new/397893873612112)

split.confirm.crash.mov
2. users list not showing on split confirm (example link: http://localhost:8080/split/new/397893873612112)

split.confirm.list.mov
3. not found view briefly appears on workspace members, invite pages

invite.mov
members.mov

@aimane-chnaif Do you think we should create separate issues for these problems or perhaps one catch-all issue/PR to handle all of them? Should I create the issue(s) or should it be someone internal from Expensify?

aimane-chnaif · 2023-05-08T10:01:32Z

@aimane-chnaif Do you think we should create separate issues for these problems or perhaps one catch-all issue/PR to handle all of them? Should I create the issue(s) or should it be someone internal from Expensify?

@amyevans will handle them

aimane-chnaif · 2023-05-10T07:59:40Z

@amyevans what's remaining before merge?

@jczekalski please fix conflict came from prettier

jczekalski · 2023-05-10T12:36:11Z

@amyevans Conflicts resolved :)

amyevans · 2023-05-10T14:23:59Z

Okay that took a lot of re-running CI jobs due to errors: GitHub Actions has encountered an internal error when running your job. (https://www.githubstatus.com/), but we're passing now.

Going to merge! 🎉 I'll create the follow up issues after I get through a few other things on my plate

OSBotify · 2023-05-11T02:43:03Z

🚀 Deployed to staging by https://github.com/amyevans in version: 1.3.13-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-05-15T08:24:18Z

🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.13-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

rushatgabhane · 2023-05-25T22:33:03Z

src/components/OptionsListSkeletonView.js

@@ -16,7 +16,7 @@ const defaultTypes = {
    shouldAnimate: true,
 };

-class LHNSkeletonView extends React.Component {
+class OptionsListSkeletonView extends React.Component {


Hi everyone! dropping a gentle note here. When we generalised this component for OptionsList, we forgot to hide the overflow of extra skeletons when there is a button at the bottom.

Causing this bug -

Good catch! Thanks for the fix

jczekalski requested a review from a team as a code owner April 12, 2023 16:49

melvin-bot bot requested review from aimane-chnaif and amyevans and removed request for a team April 12, 2023 16:49

mountiny assigned jczekalski Apr 13, 2023

jczekalski added 2 commits April 14, 2023 13:00

fix stale data issue in components that use report data and display o…

e2aec0b

…ptions list

add skeleton loading state to options list

ac71ae1

jczekalski force-pushed the jczekalski_fix/14490 branch from 14557e2 to e2aec0b Compare April 14, 2023 13:37

Merge branch 'main' into jczekalski_fix/14490

9e2ab2b

jczekalski added 2 commits April 20, 2023 12:38

resolve conflict

dfbc390

Merge branch 'main' into jczekalski_fix/14490

8a1e0d6

amyevans mentioned this pull request May 8, 2023

[HOLD for payment 2023-07-24] [$2000] Settings page: remove code preventing rendering before currentUserPersonalDetails is ready #18569

Closed

amyevans previously approved these changes May 8, 2023

View reviewed changes

aimane-chnaif previously approved these changes May 8, 2023

View reviewed changes

update with main, resolve conflicts

bbe9ebd

jczekalski dismissed stale reviews from aimane-chnaif and amyevans via bbe9ebd May 10, 2023 12:31

amyevans approved these changes May 10, 2023

View reviewed changes

aimane-chnaif approved these changes May 10, 2023

View reviewed changes

amyevans merged commit 46390d6 into Expensify:main May 10, 2023

aimane-chnaif mentioned this pull request May 10, 2023

[No QA] fix proptype warning, make prettier happy #18725

Merged

57 tasks

OSBotify mentioned this pull request May 11, 2023

Deploy Checklist: New Expensify 2023-05-11 #18764

Closed

51 tasks

PauloGasparSv mentioned this pull request May 11, 2023

[HOLD for payment 2023-05-23] [HOLD for payment 2023-05-22] Request Money - Account list for New Chat or Request money is not displayed while offline #18781

Closed

6 tasks

This was referenced May 18, 2023

[$1000] [HOLD for payment 2023-06-13] Crash on split confirm #19234

Closed

[$1000] Users list not showing on split confirm #19235

Closed

Not Found view briefly appears on workspace members and invite pages #19236

Closed

rushatgabhane reviewed May 25, 2023

View reviewed changes

fix/14490: No data displayed in options list if component using report data is rendered too early #17349

fix/14490: No data displayed in options list if component using report data is rendered too early #17349

Uh oh!

Conversation

jczekalski commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

Web

Mac

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

MelvinBot commented Apr 12, 2023

Uh oh!

github-actions bot commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jczekalski commented Apr 12, 2023

Uh oh!

jczekalski commented Apr 12, 2023

Uh oh!

aimane-chnaif commented Apr 12, 2023

Uh oh!

jczekalski commented Apr 13, 2023

Uh oh!

aimane-chnaif commented Apr 14, 2023

Uh oh!

jczekalski commented Apr 14, 2023

Uh oh!

jczekalski commented Apr 14, 2023

Uh oh!

aimane-chnaif commented Apr 18, 2023

Uh oh!

jczekalski commented Apr 19, 2023

Uh oh!

amyevans commented Apr 19, 2023

Uh oh!

aimane-chnaif commented Apr 19, 2023

Uh oh!

aimane-chnaif commented Apr 20, 2023

Uh oh!

aimane-chnaif commented Apr 20, 2023

Uh oh!

aimane-chnaif commented Apr 20, 2023

Uh oh!

jczekalski commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jczekalski commented Apr 20, 2023

Uh oh!

jczekalski commented Apr 20, 2023

Uh oh!

amyevans commented May 5, 2023

Uh oh!

jczekalski commented May 5, 2023

Uh oh!

aimane-chnaif commented May 8, 2023

Uh oh!

aimane-chnaif commented May 8, 2023

Reviewer Checklist

Screenshots/Videos

Uh oh!

jczekalski commented May 8, 2023

Uh oh!

aimane-chnaif commented May 8, 2023

Uh oh!

aimane-chnaif commented May 10, 2023

Uh oh!

jczekalski commented May 10, 2023

Uh oh!

amyevans commented May 10, 2023

Uh oh!

OSBotify commented May 11, 2023

Uh oh!

OSBotify commented May 15, 2023

Uh oh!

jczekalski commented Apr 12, 2023 •

edited

Loading

github-actions bot commented Apr 12, 2023 •

edited

Loading

jczekalski commented Apr 20, 2023 •

edited

Loading

rushatgabhane May 25, 2023 •

edited

Loading