[HOLD for payment 2023-05-22] [$4000] Not showing any data on search page when press Cmd+K shortcut while loading after login

[kavimuru]

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:

1. Update network connection to Slow 3G (to reproduce easily)
2. Login with any account
3. While full screen loading, press Cmd+K shortcut
4. Search page opens

This also applies to other pages like Profile, New chat, Send money, Workspaces, not only search

Expected Result:

• When the user launches the app and reports data is still loading, they can open the search page with CMD+K as usual
• The search page opens, and displays a skeleton UI (just as the LHN would when it doesn’t yet have reports data)
• While the search page is loading, the options in the header are not disabled. So the user can:
  • Enter a search query (still, just a skeleton UI is shown)
  • Press < to close the RHP
  • Tap outside or swipe to close the RHP
• If the reports load while the search page is displaying a skeleton UI, then any reports matching the search query are displayed. If there are no results matching the search query, then the skeleton UI is replaced by the standard No results found UI

Actual Result:

shows nothing on Search page

Workaround:

Close and re-open Search page

Platforms:

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

• MacOS / Chrome / Safari

Version Number: 1.2.58-2
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

bug.mov

Recording.76.mp4

Expensify/Expensify Issue URL:
Issue reported by: @aimane-chnaif
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1674469304602449

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013d319d96dd608a1f
• Upwork Job ID: 1620163656058773504
• Last Price Increase: 2023-03-31

[melvin-bot]

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

[mallenexpensify]

For the workaround, you can also start typing then delete text and search will populate.
I'm unsure this is a bug because there's will always(?) be a time between clicking Sign in in and being able to take any action. ie. there'll always be a loading spinner, even if it's only a fraction of a second - are all app functions that don't work in that that fraction of a second bugs?

Coincidentally... we're working on high-level testing guides for QA, I wonder if we should include "After clicking "`Sign in", app should load within x seconds (cuz... in both examples above it took multiple seconds to load)

I'm going to keep this assigned for now and unlock it for feedback

[aimane-chnaif]

@mallenexpensify this should be issue after #14456 merged.
Now we unlock all the buttons (user interactions) as well as shortcuts while showing skeleton (before loading data)
i.e. Search, Profile icons on header, New chat, New group, New workspace, etc on FAB menu.
Discussion is here.

I am happy to review this GH either Internal or External

[melvin-bot]

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

[mallenexpensify]

Thanks for pointing me to the Slack thread @aimane-chnaif , I forgot to check it :ohnothing:
@amyevans , I'm unsure if this can be External.

[amyevans]

I've updated the expected results in the OP to capture what @roryabraham shared in Slack here.

Since @aimane-chnaif has full context, I'll assign you as C+. And we can open up to external contributors.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~013d319d96dd608a1f

[melvin-bot]

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

[melvin-bot]

Current assignee @aimane-chnaif is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[s77rt]

Proposal

Part 1

diff --git a/src/CONST.js b/src/CONST.js
index 0a84a4c108..34d7a5cd02 100755
--- a/src/CONST.js
+++ b/src/CONST.js
@@ -523,6 +523,7 @@ const CONST = {
             3: 100,
         },
     },
+    OPTION_LIST_SKELETON_VIEW_ROW_HEIGHT: 60,
     EXPENSIFY_PARTNER_NAME: 'expensify.com',
     EMAIL: {
         CONCIERGE: 'concierge@expensify.com',
diff --git a/src/components/OptionsList/BaseOptionsList.js b/src/components/OptionsList/BaseOptionsList.js
index b7b6ebcd5a..aa8a60612d 100644
--- a/src/components/OptionsList/BaseOptionsList.js
+++ b/src/components/OptionsList/BaseOptionsList.js
@@ -7,6 +7,7 @@ import variables from '../../styles/variables';
 import OptionRow from '../OptionRow';
 import SectionList from '../SectionList';
 import Text from '../Text';
+import OptionsListSkeletonView from './OptionsListSkeletonView';
 import {propTypes as optionsListPropTypes, defaultProps as optionsListDefaultProps} from './optionsListPropTypes';
 
 const propTypes = {
@@ -42,13 +43,18 @@ class BaseOptionsList extends Component {
         this.didLayout = false;
 
         this.flattenedData = this.buildFlatSectionArray();
+
+        this.state = {
+            skeletonViewContainerHeight: 0,
+        };
     }
 
-    shouldComponentUpdate(nextProps) {
+    shouldComponentUpdate(nextProps, nextState) {
         return nextProps.focusedIndex !== this.props.focusedIndex
             || nextProps.selectedOptions.length !== this.props.selectedOptions.length
             || nextProps.headerMessage !== this.props.headerMessage
-            || !_.isEqual(nextProps.sections, this.props.sections)
+            || nextProps.isLoadingReportData !== this.props.isLoadingReportData
+            || nextState.skeletonViewContainerHeight !== this.state.skeletonViewContainerHeight
             || !_.isEqual(nextProps.sections, this.props.sections);
     }
 
@@ -202,36 +208,54 @@ class BaseOptionsList extends Component {
 
     render() {
         return (
-            <View style={this.props.listContainerStyles}>
-                {this.props.headerMessage ? (
-                    <View style={[styles.ph5, styles.pb5]}>
-                        <Text style={[styles.textLabel, styles.colorMuted]}>
-                            {this.props.headerMessage}
-                        </Text>
-                    </View>
-                ) : null}
-                <SectionList
-                    ref={this.props.innerRef}
-                    indicatorStyle="white"
-                    keyboardShouldPersistTaps="always"
-                    keyboardDismissMode={this.props.keyboardDismissMode}
-                    onScrollBeginDrag={this.props.onScrollBeginDrag}
-                    onScroll={this.props.onScroll}
-                    contentContainerStyle={this.props.contentContainerStyles}
-                    showsVerticalScrollIndicator={false}
-                    sections={this.props.sections}
-                    keyExtractor={this.extractKey}
-                    stickySectionHeadersEnabled={false}
-                    renderItem={this.renderItem}
-                    getItemLayout={this.getItemLayout}
-                    renderSectionHeader={this.renderSectionHeader}
-                    extraData={this.props.focusedIndex}
-                    initialNumToRender={12}
-                    maxToRenderPerBatch={5}
-                    windowSize={5}
-                    viewabilityConfig={{viewAreaCoveragePercentThreshold: 95}}
-                    onViewableItemsChanged={this.onViewableItemsChanged}
-                />
+            <View
+                style={this.props.listContainerStyles}
+                onLayout={(event) => {
+                    const skeletonViewContainerHeight = event.nativeEvent.layout.height;
+
+                    // The height can be 0 if the component unmounts - we are not interested in this value and want to know how much space it
+                    // takes up so we can set the skeleton view container height.
+                    if (skeletonViewContainerHeight === 0) {
+                        return;
+                    }
+                    this.setState({skeletonViewContainerHeight});
+                }}
+            >
+                {this.props.isLoadingReportData ? (
+                    <OptionsListSkeletonView containerHeight={this.state.skeletonViewContainerHeight} />
+                ) : (
+                    <>
+                        {this.props.headerMessage ? (
+                            <View style={[styles.ph5, styles.pb5]}>
+                                <Text style={[styles.textLabel, styles.colorMuted]}>
+                                    {this.props.headerMessage}
+                                </Text>
+                            </View>
+                        ) : null}
+                        <SectionList
+                            ref={this.props.innerRef}
+                            indicatorStyle="white"
+                            keyboardShouldPersistTaps="always"
+                            keyboardDismissMode={this.props.keyboardDismissMode}
+                            onScrollBeginDrag={this.props.onScrollBeginDrag}
+                            onScroll={this.props.onScroll}
+                            contentContainerStyle={this.props.contentContainerStyles}
+                            showsVerticalScrollIndicator={false}
+                            sections={this.props.sections}
+                            keyExtractor={this.extractKey}
+                            stickySectionHeadersEnabled={false}
+                            renderItem={this.renderItem}
+                            getItemLayout={this.getItemLayout}
+                            renderSectionHeader={this.renderSectionHeader}
+                            extraData={this.props.focusedIndex}
+                            initialNumToRender={12}
+                            maxToRenderPerBatch={5}
+                            windowSize={5}
+                            viewabilityConfig={{viewAreaCoveragePercentThreshold: 95}}
+                            onViewableItemsChanged={this.onViewableItemsChanged}
+                        />
+                    </>
+                )}
             </View>
         );
     }
diff --git a/src/components/OptionsList/OptionsListSkeletonView.js b/src/components/OptionsList/OptionsListSkeletonView.js
new file mode 100644
index 0000000000..d316cfc1c6
--- /dev/null
+++ b/src/components/OptionsList/OptionsListSkeletonView.js
@@ -0,0 +1,49 @@
+import React from 'react';
+import PropTypes from 'prop-types';
+import {Rect, Circle} from 'react-native-svg';
+import SkeletonViewContentLoader from 'react-content-loader/native';
+import themeColors from '../../styles/themes/default';
+import styles from '../../styles/styles';
+import CONST from '../../CONST';
+
+const propTypes = {
+    /** Height of the container component */
+    containerHeight: PropTypes.number.isRequired,
+
+    /** Whether to animate the skeleton view */
+    animate: PropTypes.bool,
+};
+
+const defaultProps = {
+    animate: true,
+};
+
+const OptionsListSkeletonView = (props) => {
+    // Determines the number of content items based on container height
+    const possibleVisibleContentItems = Math.ceil(props.containerHeight / CONST.OPTION_LIST_SKELETON_VIEW_ROW_HEIGHT);
+
+    const skeletonOptionsList = [];
+    for (let index = 0; index < possibleVisibleContentItems; index++) {
+        skeletonOptionsList.push(
+            <SkeletonViewContentLoader
+                animate={props.animate}
+                height={CONST.OPTION_LIST_SKELETON_VIEW_ROW_HEIGHT}
+                backgroundColor={themeColors.highlightBG}
+                foregroundColor={themeColors.border}
+                style={styles.mr5}
+                key={`skeletonOptionsList${index}`}
+            >
+                <Circle cx="40" cy="26" r="20" />
+                <Rect x="67" y="11" width="20%" height="8" />
+                <Rect x="67" y="31" width="100%" height="8" />
+            </SkeletonViewContentLoader>,
+        );
+    }
+
+    return <>{skeletonOptionsList}</>;
+};
+
+OptionsListSkeletonView.displayName = 'OptionsListSkeletonView';
+OptionsListSkeletonView.propTypes = propTypes;
+OptionsListSkeletonView.defaultProps = defaultProps;
+export default OptionsListSkeletonView;
diff --git a/src/components/OptionsList/index.js b/src/components/OptionsList/index.js
index 31128766f6..5e6d4c2fbf 100644
--- a/src/components/OptionsList/index.js
+++ b/src/components/OptionsList/index.js
@@ -1,8 +1,11 @@
 import React, {Component, forwardRef} from 'react';
 import {Keyboard} from 'react-native';
+import {withOnyx} from 'react-native-onyx';
 import _ from 'underscore';
 import BaseOptionsList from './BaseOptionsList';
 import withWindowDimensions from '../withWindowDimensions';
+import compose from '../../libs/compose';
+import ONYXKEYS from '../../ONYXKEYS';
 import {propTypes, defaultProps} from './optionsListPropTypes';
 import * as DeviceCapabilities from '../../libs/DeviceCapabilities';
 
@@ -65,7 +68,14 @@ OptionsList.propTypes = {
 };
 OptionsList.defaultProps = defaultProps;
 
-export default withWindowDimensions(forwardRef((props, ref) => (
+export default compose(
+    withWindowDimensions,
+    withOnyx({
+        isLoadingReportData: {
+            key: ONYXKEYS.IS_LOADING_REPORT_DATA,
+        },
+    }),
+)(forwardRef((props, ref) => (
     // eslint-disable-next-line react/jsx-props-no-spreading
     <OptionsList forwardedRef={ref} {...props} />
 )));
diff --git a/src/components/OptionsList/index.native.js b/src/components/OptionsList/index.native.js
index 7701096fde..b5326854e2 100644
--- a/src/components/OptionsList/index.native.js
+++ b/src/components/OptionsList/index.native.js
@@ -1,6 +1,8 @@
 import React, {forwardRef} from 'react';
 import {Keyboard} from 'react-native';
+import {withOnyx} from 'react-native-onyx';
 import BaseOptionsList from './BaseOptionsList';
+import ONYXKEYS from '../../ONYXKEYS';
 import {propTypes, defaultProps} from './optionsListPropTypes';
 
 const OptionsList = forwardRef((props, ref) => (
@@ -16,4 +18,8 @@ OptionsList.propTypes = propTypes;
 OptionsList.defaultProps = defaultProps;
 OptionsList.displayName = 'OptionsList';
 
-export default OptionsList;
+export default withOnyx({
+    isLoadingReportData: {
+        key: ONYXKEYS.IS_LOADING_REPORT_DATA,
+    },
+})(OptionsList);
diff --git a/src/components/OptionsList/optionsListPropTypes.js b/src/components/OptionsList/optionsListPropTypes.js
index 0ca8b7a709..a27bfe80f7 100644
--- a/src/components/OptionsList/optionsListPropTypes.js
+++ b/src/components/OptionsList/optionsListPropTypes.js
@@ -70,6 +70,8 @@ const propTypes = {
 
     /** Whether to show a line separating options in list */
     shouldHaveOptionSeparator: PropTypes.bool,
+
+    isLoadingReportData: PropTypes.bool,
 };
 
 const defaultProps = {
@@ -90,6 +92,7 @@ const defaultProps = {
     isDisabled: false,
     onLayout: undefined,
     shouldHaveOptionSeparator: false,
+    isLoadingReportData: false,
 };
 
 export {propTypes, defaultProps};

In this part we will handle the core feature request where I decided to implement the skeleton view globally on <OptionsList /> to cover similar issues on other places.

First I created a new skeleton component <OptionsListSkeletonView />
This skeleton view will only be shown if reports are still being loaded and for that we are going to use onyx (key: IS_LOADING_REPORT_DATA)

Basically that's all what part 1 is about. The skeleton height calculation logic is the same as on <ReportScreen /> where we use <View />'s onLayout method to get the container height and draw X rows where X = container height / height of one row

Part 2

diff --git a/src/pages/SearchPage.js b/src/pages/SearchPage.js
index 3cefc6a98c..3c0965c4a1 100755
--- a/src/pages/SearchPage.js
+++ b/src/pages/SearchPage.js
@@ -30,6 +30,8 @@ const propTypes = {
     /** All of the personal details for everyone */
     personalDetails: personalDetailsPropType.isRequired,
 
+    isLoadingReportData: PropTypes.bool,
+
     /** All reports shared with the user */
     reports: PropTypes.objectOf(reportPropTypes).isRequired,
 
@@ -44,6 +46,10 @@ const propTypes = {
     ...withLocalizePropTypes,
 };
 
+const defaultProps = {
+    isLoadingReportData: false,
+};
+
 class SearchPage extends Component {
     constructor(props) {
         super(props);
@@ -75,6 +81,13 @@ class SearchPage extends Component {
         };
     }
 
+    componentDidUpdate(prevProps) {
+        if (prevProps.isLoadingReportData === this.props.isLoadingReportData) {
+            return;
+        }
+        this.debouncedUpdateOptions();
+    }
+
     onChangeText(searchValue = '') {
         this.setState({searchValue}, this.debouncedUpdateOptions);
     }
@@ -198,11 +211,15 @@ class SearchPage extends Component {
 }
 
 SearchPage.propTypes = propTypes;
+SearchPage.defaultProps = defaultProps;
 
 export default compose(
     withLocalize,
     withWindowDimensions,
     withOnyx({
+        isLoadingReportData: {
+            key: ONYXKEYS.IS_LOADING_REPORT_DATA,
+        },
         reports: {
             key: ONYXKEYS.COLLECTION.REPORT,
         },

This part is specifically for <SearchPage /> as this page suffers from stale data. Meaning if reports gets loaded/updated after the search page is already rendered, it won't re-render to match the new reports and it will be out of sync.

As @mallenexpensify explained we have a workaround here where if we type in the search input you will get the expected results that's because changing the input will call debouncedUpdateOptions which rebuilds the list using fresh data. Fixing the stale data issue can be done in many ways as all we have to do is to call debouncedUpdateOptions.

Since we are only looking to rebuild the list if reports load after render, we can use the same onyx key we used before (IS_LOADING_REPORT_DATA).

I think the code in this part is self-explanatory.

Results

Kooha-2023-01-31-14-05-11.mp4

[amyevans]

Understanding that implementation here will ultimately be on hold for the merge of #14456, but @aimane-chnaif can you review the above proposal in the meantime?

[aimane-chnaif]

After merging #14456, there will be many pages to be opened before loading personal or reports data.
So I think this issue should be not only tied to Search page but also all possible pages including Profile, New chat, Send money, Workspaces, etc.
@amyevans what do you think?

[kavimuru]

Keyboard short cut bugs are not in scope. Closing the bug as per this comment

[aimane-chnaif]

@kavimuru this is not keyboard shortcut issue. It's not the main reproducible step though issue title says. Please re-open.

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

[mountiny]

The regression here was impossible to catch so the payments should not be degraded because of it

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

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

If no regressions arise, payment will be issued on 2023-05-22. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

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

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

[amyevans]

Bumped @mallenexpensify in New Dot for payment

[mallenexpensify]

Thanks @amyevans , @aimane-chnaif, can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~0193e6015e52379df0

@aimane-chnaif can you address the items in the BZ checklist above too please?
#14490 (comment)

[aimane-chnaif]

This bug existed from the beginning when we implement options list pages.
This was edge case first time but after implementing skeleton UI in #12698, became normal case.

Regression Test Proposal

1. Open the app and logout if already logged in
2. Set network throttling to Slow 3G or slower
3. Login any account (HT account is better to reproduce)
4. Open one of the pages as soon as the app loads:

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

5. Verify that options list displays a skeleton loading state
6. Verify that data should replace the loading state once available
7. If search term is entered while data is loading, verify that it's applied once data is displayed

[mallenexpensify]

Thanks @aimane-chnaif , TC GH is here

• https://github.com/Expensify/Expensify/issues/286289

Let me know when you've accepted in Upwork and I'll pay

[melvin-bot]

@amyevans, @mallenexpensify, @jczekalski, @aimane-chnaif Huh... This is 4 days overdue. Who can take care of this?

[mallenexpensify]

@aimane-chnaif paid $4000. I wasn't 100% sure if this is accurate but, it seemed fair upon a cursory review as the associated PR fixed a few related issues. My thinking was that any negative timeliness bonuses would be negated due to the PR fixing multiple issues. If anyone disagrees, please comment.

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

[aimane-chnaif]

Thanks @mallenexpensify
As this was agency PR, no timeline rule applied.
And this is minor but I might also be eligible for reporting bonus.

[mallenexpensify]

And this is minor but I might also be eligible for reporting bonus.

@aimane-chnaif , you def were/are. I missed it, apologies. Just paid ya $250.