[$250] Web - New Room -Console error when creating New Room

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Go to URL https://staging.new.expensify.com/

2. Login with expensify account

3. Click on FAB->New Room

4. Enter the data to create a New Room

Expected Result:

There should be no error in the console

Actual Result:

An error message appeared in the console

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web

Version Number: v1.2.14-0 Reproducible in staging?: y Reproducible in production?: y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/195621497-8449b72b-fc29-4675-b173-b03517f65475.mp4

Expensify/Expensify Issue URL: Issue reported by: Applause internal team Slack conversation:

View all open jobs on GitHub

[melvin-bot[bot]]

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

[joelbettner]

It would be good to know if there are any noticeable affects on the actual user experience, but I do agree that we should make sure we aren't throwing any errors in the console. Going to add the "external" label.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[mollfpr]

Proposal

I believe this error log happen because the API AuthenticatePusher got called before API AddWorkspaceRoom when we navigated to the report screen. It might be making the report ID not yet available on the server and responding 404 error.

https://user-images.githubusercontent.com/25520267/196019717-e31541e3-b05d-4de2-9fed-3fc20ac69a48.mov

Solution We can change the API call AddWorkspaceRoom from write to makeRequestWithSideEffects and navigate to the report screen after the promise is resolved.

diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js
index fc9b2e5a8..42ff95269 100644
--- a/src/libs/actions/Report.js
+++ b/src/libs/actions/Report.js
@@ -1255,7 +1255,8 @@ function addPolicyReport(policy, reportName, visibility) {
         },
     ];
 
-    API.write(
+    // eslint-disable-next-line rulesdir/no-api-side-effects-method
+    API.makeRequestWithSideEffects(
         'AddWorkspaceRoom',
         {
             policyID: policyReport.policyID,
@@ -1264,8 +1265,9 @@ function addPolicyReport(policy, reportName, visibility) {
             reportID: policyReport.reportID,
         },
         {optimisticData, successData},
-    );
-    Navigation.navigate(ROUTES.getReportRoute(policyReport.reportID));
+    ).then(() => {
+        Navigation.navigate(ROUTES.getReportRoute(policyReport.reportID));
+    });
 }
 
 /**

Result

https://user-images.githubusercontent.com/25520267/196019995-324e118d-a4c4-4f5f-91a4-b8b26826a8a4.mov

[melvin-bot[bot]]

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.

[thesahindia]

I believe that @mollfpr is correct about the root cause but just to be 100% sure, would you like to verify it once in the backend @dangrous?

[dangrous]

Hey @mollfpr (and @thesahindia)! So I think you're correct as to what's causing the issue - looking at the logs we're indeed getting feedback that the user doesn't have access to the newly created report. However, generally, we try to avoid using makeRequestWithSideEffects as much as possible, since it follows our attempted pattern of making every API call self-contained, and using it here would add clutter. Is there another way you can think of to avoid that error?

You probably got here but it looks like the request that's causing the issue is here

Also, just to note - In testing locally, I am not getting the same error issue, I think because any requests are basically instant (but am seeing it on prod and staging) so that's something to be aware of; we should make sure the fixes actually work the way we expect them to when there might be a delay.

cc @thienlnam for any further insight if you have it!

[thesahindia]

@dangrous, we don't have a upwork job for this.

Let's reassign @dylanexpensify

[mollfpr]

@dangrous Thanks for the feedback!

Yeah I guess you're right about Report.subscribeToReportTypingEvents(this.props.report.reportID); is causing the issue.

How about we call Report.subscribeToReportTypingEvents(this.props.report.reportID); after this.props.report.pendingFields.addWorkspaceRoom value is null?

Since the optimistic data is set to addWorkspaceRoom: "add" and the success data is addWorkspaceRoom: null.

cc @thesahindia

[mollfpr]

Proposal

Since the root cause is defined here we need to delay call of Report.subscribeToReportTypingEvents after the API command AddWorkspaceRoom is done.

Solution We can call Report.subscribeToReportTypingEvents after the FlatList is finished laying out. https://github.com/Expensify/App/blob/88515e4d22b346a197f9a698a562f15e937f6be0/src/pages/home/report/ReportActionsView.js#L349

diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js
index 06bbce049..2cc94dfd2 100755
--- a/src/pages/home/report/ReportActionsView.js
+++ b/src/pages/home/report/ReportActionsView.js
@@ -99,7 +99,6 @@ class ReportActionsView extends React.Component {
             this.setState({newMarkerSequenceNumber: 0});
         });

-        Report.subscribeToReportTypingEvents(this.props.report.reportID);
         this.keyboardEvent = Keyboard.addListener('keyboardDidShow', () => {
             if (!ReportActionComposeFocusManager.isFocused()) {
                 return;
@@ -326,6 +325,8 @@ class ReportActionsView extends React.Component {
         } else {
             Performance.markEnd(CONST.TIMING.SWITCH_REPORT);
         }
+
+        Report.subscribeToReportTypingEvents(this.props.report.reportID);
     }

     render() {

Result

https://user-images.githubusercontent.com/25520267/197009090-4479af7f-54e1-43f1-bac5-b658de10a374.mov

[dangrous]

@dylanexpensify added you back since I think you got taken off before and we don't have an upwork job for this one (thanks for noticing, @thesahindia!)

[dangrous]

Hm, yeah - moving that call later makes sense. I don't love the idea of putting it in that otherwise unrelated recordTimeToMeasureItemLayout function, feels a bit hacky. I also think it would run multiple times, which isn't ideal.

So, like I said before, testing locally is hard. BUT, I tried literally just moving the subscribe and keyboardevent block to be the last part of componentDidMount, versus in the middle, and it seems to eliminate the issue when I set my network connection to "Slow 3G". I think this is because the if statement beforehand will always fire in this situation and therefore we'll give it a bit more time to work with the Pusher command.

That is, the new code:

componentDidMount() {
    this.unsubscribeVisibilityListener = Visibility.onVisibilityChange(() => {
        if (!this.getIsReportFullyVisible()) {
            return;
        }

        // If the app user becomes active and they have no unread actions we clear the new marker to sync their device
        // e.g. they could have read these messages on another device and only just become active here
        Report.openReport(this.props.report.reportID);
        this.setState({newMarkerSequenceNumber: 0});
    });

    if (this.getIsReportFullyVisible()) {
        Report.openReport(this.props.report.reportID);
    }

    Report.subscribeToReportTypingEvents(this.props.report.reportID);
    this.keyboardEvent = Keyboard.addListener('keyboardDidShow', () => {
        if (!ReportActionComposeFocusManager.isFocused()) {
            return;
        }
        ReportScrollManager.scrollToBottom();
    });
}

What do you think - does that work locally for you as well?

[mollfpr]

I just try your @dangrous snippet code, but I still got the issue without throttling the network. AuthenticatePusher is still called before AddWorkspaceRoom and returns a 404 error.

How about using the pendingFields from the AddWorkspaceRoom API call?

[thesahindia]

What do you think - does that work locally for you as well?

Same results, it didn't work for me as well.

How about using the pendingFields from the AddWorkspaceRoom API call?

@mollfpr, feel free to share the code.

[melvin-bot[bot]]

@dangrous, @dylanexpensify, @thesahindia Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[thesahindia]

Not overdue.

[dangrous]

I think using the pendingFields could work @mollfpr ! Maybe something like wrapping the Report.subscribeToReportTypingEvents(this.props.report.reportID); in an if statement checking whether or not pendingFields.addWorkspaceRoom is null in Onyx. Only question then is, if it isn't null (which it won't be at first), how do we make sure it gets called once and only once when it is? Since componentDidMount will only run once - and too early, I'm not sure it should stay there...

Hope that helps - would love to hear what you're thinking!

[thesahindia]

@dylanexpensify, just a reminder that we don't have a Upwork job for this issue.

[dylanexpensify]

Ah, whoops! Thanks @thesahindia! Will get on it now

[dylanexpensify]

Upwork: https://www.upwork.com/jobs/~0125255e96145decb0

[mollfpr]

Sorry for the long wait @dangrous and thanks for the suggestions!

Proposal

Move the call of Report.subscribeToReportTypingEvents(this.props.report.reportID) to componentDidUpdate. We will wait for pendingFields.addWorkspaceRoom success which changes the value from add to null.

To make sure the function is called once, add a new variable this.didSubscribeToReportTypingEvents which will turn to true after the function it's called and we only call Report.subscribeToReportTypingEvents(this.props.report.reportID) when it's false.

https://github.com/Expensify/App/blob/62ff507162d7712c2aa76b5eb96141f83ec2dd91/src/pages/home/report/ReportActionsView.js#L103

diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js
index e60546bbc..d5eaf0c1c 100755
--- a/src/pages/home/report/ReportActionsView.js
+++ b/src/pages/home/report/ReportActionsView.js
@@ -67,6 +67,7 @@ class ReportActionsView extends React.Component {
         super(props);

         this.didLayout = false;
+        this.didSubscribeToReportTypingEvents = false;

         this.unsubscribeVisibilityListener = null;

@@ -100,7 +101,6 @@ class ReportActionsView extends React.Component {
             this.setState({newMarkerSequenceNumber: 0});
         });

-        Report.subscribeToReportTypingEvents(this.props.report.reportID);
         this.keyboardEvent = Keyboard.addListener('keyboardDidShow', () => {
             if (!ReportActionComposeFocusManager.isFocused()) {
                 return;
@@ -234,6 +234,12 @@ class ReportActionsView extends React.Component {
         if (didManuallyMarkReportAsUnread) {
             this.setState({newMarkerSequenceNumber: this.props.report.lastReadSequenceNumber + 1});
         }
+
+        const shouldSubscribeToReportTypingEvents = !this.props.report.pendingFields || !this.props.report.pendingFields.addWorkspaceRoom;
+        if (!this.didSubscribeToReportTypingEvents && shouldSubscribeToReportTypingEvents) {
+            Report.subscribeToReportTypingEvents(this.props.report.reportID);
+            this.didSubscribeToReportTypingEvents = true;
+        }
     }

     componentWillUnmount() {

https://user-images.githubusercontent.com/25520267/198881119-1e4fef51-673f-4968-bc8b-0ab00d0b42f8.mov

cc @dangrous @thesahindia

[dylanexpensify]

reviewing today!

[thesahindia]

~Will try to look into it in a few hours.~ I was prioritizing other issues so it took some time.

[thesahindia]

@mollfpr, it looks good to me just wanted to know do we really need to check !this.props.report.pendingFields ?

[mollfpr]

@thesahindia For the report that was already created doesn't have pendingFields or has pendingFields but not with the addWorkspaceRoom. So I guess we need that.

[thesahindia]

@mollfpr's proposal looks good to me

C+ reviewed 🎀👀🎀

@dangrous, please share your thoughts.

[dangrous]

@mollfpr this is great! I'll go ahead and assign you, if you want to apply to the Upwork job when you have a moment. Sorry for all the back and forth, and I'm happy we got here!

[melvin-bot[bot]]

📣 @mollfpr You have been assigned to this job by @dangrous! 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 📖