Expensify
HIGH: [Performance] [$500] Reliably unsubscribe from isTyping events when leaving rooms
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
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[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
Triggered auto assignment to @dylanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Job added to Upwork: https://www.upwork.com/jobs/~012a36c87d591ea59b
Triggered auto assignment to Contributor-plus team member for initial proposal review - @jjcoffee (External)
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);
};
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
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!
:ribbon::eyes::ribbon: C+ reviewed
Triggered auto assignment to @Beamanator, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
Yo yo! I'm planning to review the review tomorrow, so should be able to get someone assigned early 👍
📣 @jjcoffee 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!
📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer 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 📖
@jjcoffee PR https://github.com/Expensify/App/pull/39347 is ready to review
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 :pray:
@jjcoffee do you mean taht @tienifr should re-review? @tienifr can you please do this today, or what's your ETA?
@tienifr - I see @Beamanator responded. What are the next steps, and your ETA?
It is delayed because I need time to check if the new changes work and do not cause errors.
Yes sorry for not updating here! Was merged 3 days ago, on staging as of 7 hours ago 🙏 🚀
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