search

mattermost / desktop

Mattermost Desktop application for Windows, Mac and Linux
Apache License 2.0
2k stars 816 forks source link

[MM-59489] Change logged in tracking code for new API, check for `/` when navigating tabs #3105

Closed devinbinnie closed 2 months ago

devinbinnie commented 2 months ago

Summary

One of the changes to the Desktop API was that individual tabs report their login change directly to the Desktop App instead of relying on local storage to do so. At some point it appears that localStorage was not syncing to inactive tabs, which makes sense given that that doesn't happen on Chrome either unless the tab is focused. This caused no issue on web app, but the navigation code on Desktop App was effected in that if you logged out of another tab, logged back in on the main tab and tried to navigate to the other tab using a link of some kind, the other tab would try to load and not be logged in correctly before it tried to navigate, causing a session expiry.

This PR fixes the log in tracking code for the new API, forcing each individual tab to reload if necessary (like the old code, but this time just the individual tab is responsible for forcing the reload), and also checks to make sure that on logout, we go back to the Channels tab, since it seems like that wasn't always happening either.

Ticket Link

https://mattermost.atlassian.net/browse/MM-59489

Fixed an issue where logging out from the Boards/Playbooks tabs and trying to navigate after logging back in would force an unexpected logout
hmhealey commented 2 months ago

At some point it appears that localStorage was not syncing to inactive tabs, which makes sense given that that doesn't happen on Chrome either unless the tab is focused.

Does this also mean the same bug might apply on the web app as well? It sort of makes sense that it wouldn't want to wake up every tab on a given domain whenever localStorage changes, but I wasn't aware that Chrome had this behaviour.

I don't know if that would be the case though because the web app sort of implicitly tracks if its logged in based on the MMUSERID and MMAUTHTOKEN cookies. The localStorage method is only used because the web app can't access the cookie directly, and we wanted to give it a kick to have it realize it had been logged in (and we didn't know postMessage existed at the time apparently)

devinbinnie commented 2 months ago

At some point it appears that localStorage was not syncing to inactive tabs, which makes sense given that that doesn't happen on Chrome either unless the tab is focused.

Does this also mean the same bug might apply on the web app as well? It sort of makes sense that it wouldn't want to wake up every tab on a given domain whenever localStorage changes, but I wasn't aware that Chrome had this behaviour.

I don't know if that would be the case though because the web app sort of implicitly tracks if its logged in based on the MMUSERID and MMAUTHTOKEN cookies. The localStorage method is only used because the web app can't access the cookie directly, and we wanted to give it a kick to have it realize it had been logged in (and we didn't know postMessage existed at the time apparently)

This won't affect the web app as a bug because the tabs still receive the signal, they just don't seem to act on it until the tab gets focused. That must have changed at some point (not sure if it was us or Chrome that did that) but it should affect the web app since there's no forwarding logic between the two.