[GH-371] Fixed issue: Getting multiple DMs when Oauth token is revoked #408
Conversation
…ed (#42) * [MI-3561] Fixed issue: Getting multiple DMs when Oauth token is revoked - Created a single API to get the data for LHS and todo command. * [MI-3561] Review fixes 1. Replaced 'babel-eslint' package with '@babel/eslint-parser' as the previous one was deprecated * [MI-3561] Review fixes * [MI-3561] Review fix
raghavaggarwal2308
requested review from
mickmister and
spirosoik
as code owners
There was a problem hiding this comment.
LGTM. Great improvement for consolidating LHS data into one API request as well
One thing that comes to mind is the behavior when there are multiple tabs of Mattermost open. For instance, the desktop app his 3 instances of the Mattermost webapp running at once.
| this.props.actions.getYourPrs(), | ||
| this.props.actions.getYourAssignments(), | ||
| ]); | ||
| await this.props.actions.getLHSData(); |
There was a problem hiding this comment.
Any thoughts on error handling here?
There was a problem hiding this comment.
@mickmister In the left-hand sidebar we will not have any place to display the error. I think the only place we will be able to display the error will be the RHS. Currently, we are just logging the error in the server logs. Do you have any suggestions on it?
There was a problem hiding this comment.
The main thing I noticed is that we call this.setState({refreshing: false}) here, which will not run if an error happens. Even if it's as simple as logging the error, I think we should have some sort of error handling there to make sure we still clean up the state after the call.
@mickmister I am not sure what you are asking here. Can you please add some details? |
|
@raghavaggarwal2308 When multiple tabs of Mattermost are open, they all receive the same websocket messages. It's possible two browsers call the LHS data endpoint at the same time because of the websocket message syncing. It's probably unlikely for them to be that simultaneous, enough to not take it into account, but it's still worth bringing up for discussion since we're changing how the LHS data works for the purpose of avoiding race conditions. If you think there are no issues, then that's good with me |
|
@mickmister I have tested what you are saying for the following cases:
Everything was working fine. So, I don't think it will be an issue. |
|
@raghavaggarwal2308 A server restart would be another reason why each tab needs to contact the server again, though reconnecting the account should do so as well. I think no further testing is needed here on your part |
Summary
Screenshots
Possible risks:
What to test?
Steps to reproduce:
Environment:
MM version: v7.8.2
Node version: 14.18.0
Go version: 1.19.0
Ticket Link
Fixes #371