Do not re-subscribe to Pusher when clicking on desktop notification

[marcaaron]

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:

Clicking on a desktop web (Chrome) notification

Expected Result:

Does not unsubscribe then re-subscribe to Pusher

Actual Result:

It seems to always unsubscribe and resubscribe from Pusher

[Pusher] Unsubscribing from channel true {channelName: "private-report-reportID-64708287"}
pusher.js:196 [Pusher] Attempting to subscribe to channel true {channelName: "private-report-reportID-64708287", eventName: "client-userIsTyping"}

Workaround:

Just use the app as normal. This is indicative of reloading the chat unnecessarily.

Platform:

Web
iOS
Android
Desktop App
Mobile Web

Version Number:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on Upwork

[MelvinBot]

Triggered auto assignment to @Christinadobrzyn (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[marcaaron]

My best guess is that this issue is related to how we are navigating to a report.

https://github.com/Expensify/Expensify.cash/blob/e78340b3e9a3e4fea6dd3f75dcbc32b79c63cc7b/src/libs/actions/Report.js#L618-L624

https://github.com/Expensify/Expensify.cash/blob/e78340b3e9a3e4fea6dd3f75dcbc32b79c63cc7b/src/libs/Navigation/Navigation.js#L58-L64

If we are already on the report we should either reveal the report or do nothing rather than "navigate" to it (which might be reloading it). Something else could be going on but that's what I'd check out first.

[Christinadobrzyn]

hey @marcaaron Testing this to better understand what needs to be fixed.

• sign-in in to e.cash at www.expensify.cash in Chome browser
• Allow Chrome browser notifications for www.expensify.cash
• Start convo with someone
• When I get a push notification > check the chat to see if the chat has loaded to match the recent message
• Chat loaded (with delay) to show the recent message > I couldn't see anything in the console but I might not be using it right 😅

Will you confirm the issue we want to fix is the delayed upload of the most recent message in a chat?

[dklymenk]

Hello, I am not 100% certain about logs referenced in the first post, but here is how I reproduced the issue:

1. Login into account A and account B from separate browsers/tabs.
2. Send a message from account A to B.
3. Wait a few seconds.
4. Open tab/browser with account B.
5. Accept the notification permission modal.
6. Go back to account A browser and send a message.
7. Activate the notification.
8. A browser/tab with account B gets opened. You can see messages appearing, being redrawn. It looks like the ReportView gets reloaded/remounted just like @marcaaron said.
9. If you look at messages sent over WS via dev tools, you'll see event pusher:unsubscribe immediately followed by pusher:subscribe.

Here is quick reproduction demo video (steps 6-9):

3893-repro.mp4

Proposal

And just as @marcaaron said, the fix is as simple as preventing the navigation if requested route is the current one:

--- a/src/libs/Navigation/Navigation.js
+++ b/src/libs/Navigation/Navigation.js
@@ -58,7 +58,8 @@ function navigate(route = ROUTES.HOME) {
     // Navigate to the ReportScreen with a custom action so that we can preserve the history. We're looking to see if we
     // have a participants route since those should go through linkTo() as they open a different screen.
     const {reportID, isParticipantsRoute} = ROUTES.parseReportRouteParams(route);
-    if (reportID && !isParticipantsRoute) {
+
+    if (reportID && !isParticipantsRoute && navigationRef.current.getCurrentRoute().path !== `/${route}`) {
         navigationRef.current.dispatch(CustomActions.pushDrawerRoute(SCREENS.REPORT, {reportID}));
         return;
     }

Here is a video with a fix working (no unsubscribe event, no chat redrawing):

3893-fix.mp4

Thanks.

[rdjuric]

@marcaaron I don't think there's a way to keep the history in our Drawer without reseting the navigation state. We could try:

• Nesting a StackNavigator in our DrawerNavigator
• Writing a custom router based on the Drawer router that deals differently with the history.

But I'm not convinced it's worth it, our implementation works well apart from the remount.

---

To fix this one we have to skip the reset when we're navigating to the same report.

1. We start passing the navigationRef to our pushDrawerRoute here. We'll need it to dispatch multiple actions and to get the RootState.
2. In our pushDrawerRoute, get our active report using navigationRef.current.getRootState(). We can't use the state from the dispatch here since if there's an open modal above the report the dispatch state will be from the modal.
3. If our active report is the same as the one we're trying to push we:
   • Dispatch StackActions.pop() if our state.type != 'drawer'
   • Dispatch DrawerActions.closeDrawer().
     I don't think we have to check for smallScreenWidth before closing the drawer, it doesn't have any effect on big screens.

[marcaaron]

@dklymenk that proposal is half of the story as we need to control the drawer + dismiss any modals

But I'm not convinced it's worth it, our implementation works well apart from the remount.

@rdjuric I'm inclined to agree with you here. And the proposal sounds like it's moving in the right direction (perhaps we can find a better way still in the review). I do wish there was a simpler way, but keeping some semblance of browser history is also a priority for us.

[rdjuric]

PR is up at #3945!

Do you want to take a look at it @marcaaron? Pullerbear requested a review from someone else 😅

[Christinadobrzyn]

I think I missed the step of adding the Eng label to this so I'm going to do that so we can get another engineer in here just in case Marc is busy.

[MelvinBot]

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

[Christinadobrzyn]

Created Upwork Job
External: https://www.upwork.com/jobs/~01990e714d0d1f8b93
Internal: https://www.upwork.com/ab/applicants/1413362641408704512/job-details

[Christinadobrzyn]

Hey there @nickmurray47 or @marcaaron can you let me know if the PR looks good for this project? Thanks!

[nickmurray47]

Taking a look now!

[nickmurray47]

The proposal looks good to me as far as I can tell.

I had the same kinda nitpicky concern that @rdjuric brought up with the hard-coded line 'routes[0].state.routes[0].params.reportID'. I'd much rather have that in some kind of getActiveReport(rootState) like you mentioned.

[marcaaron]

It looks like the PR has been merged already?

[Christinadobrzyn]

PR was merged, holding for 7 days to ensure no regressions and will pay in Upwork on Friday July 16 (given the 7th day is a Saturday)

[rdjuric]

@Christinadobrzyn Thanks, Christina! Just a note that the PR was merged 3 days ago

@nickmurray47 nice, I'll open another PR to improve that line later

[Christinadobrzyn]

@rdjuric is paid in Upwork! Thank you for working on this!