[$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 contributors@expensify.com 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:

Bug5775247_Recording__2351.mp4

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

View all open jobs on GitHub

[melvin-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]

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

[melvin-bot]

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

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

Screen.Recording.2022-10-16.at.12.29.31.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

Screen.Recording.2022-10-16.at.12.35.46.mov

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

App/src/pages/home/report/ReportActionsView.js

Line 349 in 88515e4

onLayout={this.recordTimeToMeasureItemLayout}

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

Screen.Recording.2022-10-20.at.23.42.33.mov

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

📣 @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 📖

[mollfpr]

@dangrous @thesahindia PR raised 🚀

I also added the fix for a similar issue when creating New Chat / New Group Chat. So not just checking pendingFields.addWorkspaceRoom but also pendingFields.createChat for optimistic success when creating the new chat.

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist will need to be completed before the issue can be closed:

• A regression test has been added or updated so that the same bug will not reach production again. Link to the updated test here: https://github.com/Expensify/Expensify/issues/241908
• The PR that introduced the bug has been identified. Link to the PR:
• 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:
• 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: https://expensify.slack.com/archives/C02NK2DQWUX/p1668005382430089
• Payment has been made to the issue reporter (if applicable)
• Payment has been made to the contributor that fixed the issue (if applicable)
• Payment has been made to the contributor+ that helped on the issue (if applicable)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist will need to be completed before the issue can be closed:

• A regression test has been added or updated so that the same bug will not reach production again. Link to the updated test here:
• The PR that introduced the bug has been identified. Link to the PR:
• 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:
• 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:
• Payment has been made to the issue reporter (if applicable)
• Payment has been made to the contributor that fixed the issue (if applicable)
• Payment has been made to the contributor+ that helped on the issue (if applicable)

[dangrous]

Great job @mollfpr! I just merged.

[dylanexpensify]

@thesahindia @mollfpr @dangrous have we seen any regressions with this?

[mollfpr]

@dylanexpensify We haven’t. The PR is still in QA.

[dylanexpensify]

Thanks @mollfpr! @dangrous any idea how long the PR will be in QA?

[dangrous]

I... do not, unfortunately. I know they're working on some deploy issues so it might be a bit before it gets up.

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[dangrous]

The PR has been merged into prod (not sure why we didn't get the notification here. But the 7 day clock can start, and looks like from that ^ we're off to a good start.

[dylanexpensify]

@thesahindia @mollfpr can you both please apply to the above upwork job? 🙇

[mollfpr]

@dylanexpensify Applied, thank you!

[thesahindia]

Done!

[thesahindia]

@dylanexpensify, this is ready for the payment.

[mvtglobally]

Issue not reproducible during KI retests. (Second week)

[mollfpr]

Bump @dylanexpensify 🚀

[dylanexpensify]

Sorry y'all! Getting payment sorted this AM!

[dylanexpensify]

Offers sent @thesahindia @mollfpr

[mollfpr]

@dylanexpensify Applied, thank you!

[thesahindia]

@dylanexpensify, accepted 🚀

[dylanexpensify]

paid out, contracts closed! Appreciate y'all!