Refactor "new action" logic in ReportActionsView to fix new marker

package.json

@@ -97,7 +97,7 @@
     "react-native-image-picker": "^4.8.5",
     "react-native-image-size": "git+https://github.com/Expensify/react-native-image-size#6b5ab5110dc3ed554f8eafbc38d7d87c17147972",
     "react-native-modal": "^13.0.0",
-    "react-native-onyx": "1.0.26",
+    "react-native-onyx": "1.0.27",
     "react-native-pdf": "^6.6.2",
     "react-native-performance": "^2.0.0",
     "react-native-permissions": "^3.0.1",

src/ONYXKEYS.js

@@ -155,6 +155,9 @@ export default {
     // Is report data loading?
     IS_LOADING_REPORT_DATA: 'isLoadingReportData',
 
+    // Are we loading the initial app data?
+    IS_LOADING_INITIAL_APP_DATA: 'isLoadingInitialAppData',
+
     // Is Keyboard shortcuts modal open?
     IS_SHORTCUTS_MODAL_OPEN: 'isShortcutsModalOpen',
 

src/libs/Middleware/SaveResponseInOnyx.js

@@ -1,5 +1,4 @@
 import Onyx from 'react-native-onyx';
-import _ from 'underscore';
 
 /**
  * @param {Promise} response
@@ -9,28 +8,26 @@ import _ from 'underscore';
 function SaveResponseInOnyx(response, request) {
     return response
         .then((responseData) => {
-            const onyxUpdates = [];
-
             // Make sure we have response data (i.e. response isn't a promise being passed down to us by a failed retry request and responseData undefined)
             if (!responseData) {
                 return;
             }
 
-            // Handle the request's success/failure data (client-side data)
-            if (responseData.jsonCode === 200 && request.successData) {
-                onyxUpdates.push(...request.successData);
-            } else if (responseData.jsonCode !== 200 && request.failureData) {
-                onyxUpdates.push(...request.failureData);
-            }
+            // First apply any onyx data updates that are being sent back from the API. We wait for this to complete and then
+            // apply successData or failureData. This ensures that we do not update any pending, loading, or other UI states contained
+            // in successData/failureData until after the component has received and API data.
+            const onyxDataUpdatePromise = responseData.onyxData
+                ? Onyx.update(responseData.onyxData)
+                : Promise.resolve();
 
-            // Add any onyx updates that are being sent back from the API
-            if (responseData.onyxData) {
-                onyxUpdates.push(...responseData.onyxData);
-            }
-
-            if (!_.isEmpty(onyxUpdates)) {
-                Onyx.update(onyxUpdates);
-            }
+            onyxDataUpdatePromise.then(() => {
+                // Handle the request's success/failure data (client-side data)
+                if (responseData.jsonCode === 200 && request.successData) {
+                    Onyx.update(request.successData);
+                } else if (responseData.jsonCode !== 200 && request.failureData) {
+                    Onyx.update(request.failureData);
+                }
+            });
 
             return responseData;
         });

src/libs/actions/App.js

@@ -117,21 +117,42 @@ AppState.addEventListener('change', (nextAppState) => {
  */
 function openApp() {
     API.read('OpenApp', {policyIDList}, {
-        optimisticData: [{
-            onyxMethod: CONST.ONYX.METHOD.MERGE,
-            key: ONYXKEYS.IS_LOADING_REPORT_DATA,
-            value: true,
-        }],
-        successData: [{
-            onyxMethod: CONST.ONYX.METHOD.MERGE,
-            key: ONYXKEYS.IS_LOADING_REPORT_DATA,
-            value: false,
-        }],
-        failureData: [{
-            onyxMethod: CONST.ONYX.METHOD.MERGE,
-            key: ONYXKEYS.IS_LOADING_REPORT_DATA,
-            value: false,
-        }],
+        optimisticData: [
+            {
+                onyxMethod: CONST.ONYX.METHOD.MERGE,
+                key: ONYXKEYS.IS_LOADING_REPORT_DATA,
+                value: true,
+            },
+            {
+                onyxMethod: CONST.ONYX.METHOD.MERGE,
+                key: ONYXKEYS.IS_LOADING_INITIAL_APP_DATA,
+                value: true,
+            },
+        ],
+        successData: [
+            {
+                onyxMethod: CONST.ONYX.METHOD.MERGE,
+                key: ONYXKEYS.IS_LOADING_REPORT_DATA,
+                value: false,
+            },
+            {
+                onyxMethod: CONST.ONYX.METHOD.MERGE,
+                key: ONYXKEYS.IS_LOADING_INITIAL_APP_DATA,
+                value: false,
+            },
+        ],
+        failureData: [
+            {
+                onyxMethod: CONST.ONYX.METHOD.MERGE,
+                key: ONYXKEYS.IS_LOADING_REPORT_DATA,
+                value: false,
+            },
+            {
+                onyxMethod: CONST.ONYX.METHOD.MERGE,
+                key: ONYXKEYS.IS_LOADING_INITIAL_APP_DATA,
+                value: false,
+            },
+        ],
     });
 }
 

src/libs/actions/Report.js

@@ -1384,6 +1384,27 @@ function setIsComposerFullSize(reportID, isComposerFullSize) {
     Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_IS_COMPOSER_FULL_SIZE}${reportID}`, isComposerFullSize);
 }
 
+const defaultNewActionSubscriber = {
+    reportID: '',
+    callback: () => {},
+};
+
+let newActionSubscriber = defaultNewActionSubscriber;
+
+/**
+ * Enables the Report actions file to let the ReportActionsView know that a new comment has arrived in realtime for the current report
+ *
+ * @param {String} reportID
+ * @param {Function} callback
+ * @returns {Function}
+ */
+function subscribeToNewActionEvent(reportID, callback) {
+    newActionSubscriber = {callback, reportID};
+    return () => {
+        newActionSubscriber = defaultNewActionSubscriber;
+    };
+}
+
 /**
  * @param {String} reportID
  * @param {Object} action
@@ -1398,7 +1419,10 @@ function viewNewReportAction(reportID, action) {
     if (isFromCurrentUser) {
         updatedReportObject.lastVisitedTimestamp = Date.now();
         updatedReportObject.lastReadSequenceNumber = action.sequenceNumber;
-        updatedReportObject.maxSequenceNumber = action.sequenceNumber;
+    }
+
+    if (reportID === newActionSubscriber.reportID) {
+        newActionSubscriber.callback(isFromCurrentUser, updatedReportObject.maxSequenceNumber);
     }
 
     Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, updatedReportObject);
@@ -1478,12 +1502,6 @@ Onyx.connect({
                 return;
             }
 
-            // We don't want to process any new actions that have a pendingAction field as this means they are "optimistic" and no notifications
-            // should be created for them
-            if (!_.isEmpty(action.pendingAction)) {
-                return;
-            }
-
             if (!action.timestamp) {
                 return;
             }
@@ -1536,4 +1554,5 @@ export {
     clearPolicyRoomNameErrors,
     clearIOUError,
     getMaxSequenceNumber,
+    subscribeToNewActionEvent,
 };

src/pages/home/ReportScreen.js

@@ -146,20 +146,20 @@ class ReportScreen extends React.Component {
     }
 
     /**
-     * When reports change there's a brief time content is not ready to be displayed
-     * It Should show the loader if it's the first time we are opening the report
+     * When false the ReportActionsView will completely unmount and we will show a loader until it returns true.
      *
      * @returns {Boolean}
      */
-    shouldShowLoader() {
+    isReportReadyForDisplay() {
         const reportIDFromPath = getReportID(this.props.route);
 
-        // This means there are no reportActions at all to display, but it is still in the process of loading the next set of actions.
-        const isLoadingInitialReportActions = _.isEmpty(this.props.reportActions) && this.props.report.isLoadingReportActions;
+        // We need to wait for the initial app data to load as it tells us which reports are unread. Waiting will allow the new marker to be set
+        // correctly when the report mounts. If we don't do this, the report may initialize in the "read" state and the marker won't be set at all.
+        const isLoadingInitialAppData = this.props.isLoadingInitialAppData;
 
         // This is necessary so that when we are retrieving the next report data from Onyx the ReportActionsView will remount completely
         const isTransitioning = this.props.report && this.props.report.reportID !== reportIDFromPath;
-        return !reportIDFromPath || isLoadingInitialReportActions || !this.props.report.reportID || isTransitioning;
+        return reportIDFromPath && this.props.report.reportID && !isTransitioning && !isLoadingInitialAppData;
     }
 
     fetchReportIfNeeded() {
@@ -217,6 +217,9 @@ class ReportScreen extends React.Component {
         const addWorkspaceRoomOrChatPendingAction = lodashGet(this.props.report, 'pendingFields.addWorkspaceRoom') || lodashGet(this.props.report, 'pendingFields.createChat');
         const addWorkspaceRoomOrChatErrors = lodashGet(this.props.report, 'errorFields.addWorkspaceRoom') || lodashGet(this.props.report, 'errorFields.createChat');
         const screenWrapperStyle = [styles.appContent, styles.flex1, {marginTop: this.state.viewportOffsetTop}];
+
+        // There are no reportActions at all to display and we are still in the process of loading the next set of actions.
+        const isLoadingInitialReportActions = _.isEmpty(this.props.reportActions) && this.props.report.isLoadingReportActions;
         return (
             <Freeze
                 freeze={this.props.isSmallScreenWidth && this.props.isDrawerOpen}
@@ -280,32 +283,35 @@ class ReportScreen extends React.Component {
                                 this.setState({skeletonViewContainerHeight});
                             }}
                         >
-                            {this.shouldShowLoader()
-                                ? (
-                                    <ReportActionsSkeletonView
-                                        containerHeight={this.state.skeletonViewContainerHeight}
-                                    />
-                                )
-                                : (
-                                    <>
-                                        <ReportActionsView
-                                            reportActions={this.props.reportActions}
-                                            report={this.props.report}
-                                            session={this.props.session}
-                                            isComposerFullSize={this.props.isComposerFullSize}
-                                            isDrawerOpen={this.props.isDrawerOpen}
-                                        />
-                                        <ReportFooter
-                                            errors={addWorkspaceRoomOrChatErrors}
-                                            pendingAction={addWorkspaceRoomOrChatPendingAction}
-                                            isOffline={this.props.network.isOffline}
-                                            reportActions={this.props.reportActions}
-                                            report={this.props.report}
-                                            isComposerFullSize={this.props.isComposerFullSize}
-                                            onSubmitComment={this.onSubmitComment}
-                                        />
-                                    </>
-                                )}
+                            {this.isReportReadyForDisplay() && (
+                                <ReportActionsView
+                                    reportActions={this.props.reportActions}
+                                    report={this.props.report}
+                                    session={this.props.session}
+                                    isComposerFullSize={this.props.isComposerFullSize}
+                                    isDrawerOpen={this.props.isDrawerOpen}
+                                    parentViewHeight={this.state.skeletonViewContainerHeight}
+                                />
+                            )}
+
+                            {/* Note: The report should be allowed to mount even if the initial report actions are not loaded. If we prevent rendering the report while they are loading then
+                            we'll unnecessarily unmount the ReportActionsView which will clear the new marker lines initial state. */}
+                            {(!this.isReportReadyForDisplay() || isLoadingInitialReportActions) && (
+                                <ReportActionsSkeletonView
+                                    containerHeight={this.state.skeletonViewContainerHeight}
+                                />
+                            )}
+                            {this.isReportReadyForDisplay() && !isLoadingInitialReportActions && (
+                                <ReportFooter
+                                    errors={addWorkspaceRoomOrChatErrors}
+                                    pendingAction={addWorkspaceRoomOrChatPendingAction}
+                                    isOffline={this.props.network.isOffline}
+                                    reportActions={this.props.reportActions}
+                                    report={this.props.report}
+                                    isComposerFullSize={this.props.isComposerFullSize}
+                                    onSubmitComment={this.onSubmitComment}
+                                />
+                            )}
                         </View>
                     </FullPageNotFoundView>
                 </ScreenWrapper>
@@ -351,5 +357,8 @@ export default compose(
         personalDetails: {
             key: ONYXKEYS.PERSONAL_DETAILS,
         },
+        isLoadingInitialAppData: {
+            key: ONYXKEYS.IS_LOADING_INITIAL_APP_DATA,
+        },
     }),
 )(ReportScreen);