[Awaiting Payment April 29th] HIGH: [Performance] [$500] Reliably unsubscribe from isTyping events when leaving rooms

[quinthar]

Problem:

When you join a room, you subscribe to various Pusher channels that should only be active while in that room. However, when you leave the room, sometimes it doesn't unsubscribe you from the channels. This "leak" will gradually slow down the app. But particularly concerning is that it will cause you to receive Pusher events that do not relate to the current room (such as "istyping" events in rooms you aren't looking at), and thus drag down your performance.

Solution:

Reliably unsubscribe from the room's Pusher channel when leaving by any method. There is a theory that we do this when leaving a room to switch to another, but perhaps not when leaving the room to go to a non-room page (eg, Settings).

See: https://expensify.slack.com/archives/C049HHMV9SM/p1711131919203929

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012a36c87d591ea59b
• Upwork Job ID: 1773645194916745216
• Last Price Increase: 2024-03-29
• Automatic offers:
• jjcoffee | Reviewer | 0
• tienifr | Contributor | 0

[melvin-bot]

Auto-assigning issues to engineers is no longer supported. If you think this issue should receive engineering attention, please raise it in #whatsnext.

[melvin-bot]

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

[dylanexpensify]

Nice! @quinthar is this something we wanna get a contributor on?

[quinthar]

yes please!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~012a36c87d591ea59b

[melvin-bot]

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

[dylanexpensify]

Let's get it!

[ShridharGoel]

Proposal

Please re-state the problem that we are trying to solve in this issue.

We need to unsubscribe from isTyping events when leaving rooms.

What is the root cause of that problem?

Pusher remains subscribed even after leaving the room.

What changes do you think we should make in order to solve the problem?

In ReportActionsView.ts, we are calling Report.subscribeToReportTypingEvents(reportID);.

We can call the below to unsubscribe in a useEffect when component unmounts:

// Unsubscribe from Pusher events when component unmounts
return () => {
    Report.unsubscribeFromReportTypingEvents(reportID);
};

[tienifr]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When you join a room, you subscribe to various Pusher channels that should only be active while in that room. However, when you leave the room, sometimes it doesn't unsubscribe you from the channels. This "leak" will gradually slow down the app.

What is the root cause of that problem?

In here, we already have the logic to unsubscribe from the isTyping event. However this only happens when the report screen is unmounted, it won't happen if the report screen simply loses focus (is no longer the topmost report)

What changes do you think we should make in order to solve the problem?

We need to unsubscribe from report channel if the screen loses focus, and the reportId is no longer the topmostReportId.

Then we'll resubscribe if the report screen gains focus.

We might want to do it in ReportActionsView because that's where we're subscribing to the event, but moving both to ReportActionsList is also fine.

Pseudo-code (to replace this event subscription logic, some more polishing might be needed)

useEffect(() => {
       if (route?.params?.reportID !== reportID) {
           return;
       }
       
       if (isFocused) {
           const didCreateReportSuccessfully = !report.pendingFields || (!report.pendingFields.addWorkspaceRoom && !report.pendingFields.createChat);
           
           if (!didSubscribeToReportTypingEvents.current && didCreateReportSuccessfully) {
               const interactionTask = InteractionManager.runAfterInteractions(() => {
                   Report.subscribeToReportTypingEvents(reportID);
                   didSubscribeToReportTypingEvents.current = true;
               });
               return () => {
                   interactionTask.cancel();
               };
           }
       } else {
           const topmostReportId = Navigation.getTopmostReportId();

           if (topmostReportId !== report.reportID && didSubscribeToReportTypingEvents.current) {
               didSubscribeToReportTypingEvents.current = false;
               InteractionManager.runAfterInteractions(() => {
                   Report.unsubscribeFromReportChannel(report.reportID);
               });
           }
       }
   }, [isFocused, report.reportID, report.pendingFields, didSubscribeToReportTypingEvents, route, reportID, prevIsFocused]);

Then we can remove the existing unsubscription logic here

What alternative solutions did you explore? (Optional)

NA

[jjcoffee]

I like the sound of @tienifr's proposal! The RCA looks to be correct, since we already unsubscribe on unmount (which is why it only sometimes sends the isTyping events). The solution also seems decent from a quick test run, e.g. navigating to the settings page correctly unsubscribes, whereas opening the room details modal does not unsubscribe (so you'd still see the ... is typing messages in the background).

@ShridharGoel's proposal is lacking an RCA and generally in details, for example the solution refers to a non-existent method unsubscribeFromReportTypingEvents and details of where code changes are being proposed to be made are missing. I recommend working on fleshing out your proposals with plenty of detail for the future. Best of luck!

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[tienifr]

@jjcoffee PR #39347 is ready to review

[jjcoffee]

There are a couple of extra cases I've found that we need to handle on the PR. Waiting on @tienifr to look into it 🙏

[quinthar]

@jjcoffee do you mean taht @tienifr should re-review? @tienifr can you please do this today, or what's your ETA?

[tienifr]

@quinthar There is a case we need the confirmation from internal team. Asked here

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[quinthar]

@tienifr - I see @Beamanator responded. What are the next steps, and your ETA?

[tienifr]

@quinthar I updated the PR (See comment).

[tienifr]

It is delayed because I need time to check if the new changes work and do not cause errors.

[quinthar]

What's the update here?

[quinthar]

oh I see it's merged!

[Beamanator]

Yes sorry for not updating here! Was merged 3 days ago, on staging as of 7 hours ago 🙏 🚀

[quinthar]

Can we close this?

[Beamanator]

Hmm this has been in prod for only 3 days now, so i don't believe we should close - we normally wait 7 days before payment & before considering the PR having caused no regressions

[quinthar]

Should this be tagged Awaiting Payment then?

[Beamanator]

Aah true, I think that automation wasn't working one day last week, so I manually updated 👍

@dylanexpensify FYI looks like payment is due today! 😅

[dylanexpensify]

Wahooo! Will do it!

[dylanexpensify]

Payment summary:

• Contributor: @tienifr $500
• Contributor+: @jjcoffee $500

Please apply/request!

[dylanexpensify]

done! @jjcoffee can we get checklist sorted?

[jjcoffee]

• The PR that introduced the bug has been identified. Link to the PR: N/A - this behaviour was always present
• 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: N/A
• 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: N/A
• Determine if we should create a regression test for this bug. Yes
• 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.

Regression Test Proposal

1. Device A: Sign in with user A and open report with user B
2. Device B: Sign in with user B and open report with user A
3. Device A: Type anything in composer
4. Device B: Verify that there is a text "User A is typing..." appears below the composer for a while
5. Device B: Open another report
6. Device A: Type anything in composer
7. Device B: Open developer mode, choose "Console" tab. Verify that there is no reportUserIsTyping... log in console, e.g. [info] [Onyx] merge() called for key: reportUserIsTyping_4555629044294735 properties: 15225462 - ""

Do we agree 👍 or 👎

[melvin-bot]

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

[dylanexpensify]

nice!

[dylanexpensify]

done!