Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notification-open logic assumes active account has server data #5703

Open
gnprice opened this issue Mar 30, 2023 · 0 comments
Open

Notification-open logic assumes active account has server data #5703

gnprice opened this issue Mar 30, 2023 · 0 comments

Comments

@gnprice
Copy link
Member

gnprice commented Mar 30, 2023

We see some stack traces in Sentry that involve a "No server data found" error on this getOwnUserId call in narrowToNotification:

const narrow = getNarrowFromNotificationData(
data,
getAllUsersByEmail(state),
getStreamsByName(state),
getOwnUserId(state),

I believe what this means for the user is: they tried to open a notification, the app launched, and then it just went to the inbox screen and forgot about navigating to the conversation the notification was from.

The bug is that this code is effectively assuming we have server data, but what it actually knows at this point is only that the account the notification is for is the active account. (Or that we couldn't tell what account the notification was for, so that getAccountFromNotificationData returned null, and we're proceeding on the optimistic assumption that it's the active account.)

In reality, the active account may not have server data. This state happens naturally any time we have a RESET_ACCOUNT_DATA action, until we have a subsequent REGISTER_COMPLETE. In particular it happens

  • on a login or account-switch, until the subsequent register completes;
  • on a logout, until and unless the user selects another account;
  • on startup, if we were in such a state when we last saved data in a previous run.

So the logic should handle that. Ideally we'd (a) confirm we're in a state where we're trying to load server data, and then (b) wait for it to arrive. But we don't currently have an obvious way to do (b) — this is basically the same obstacle that's in the way of #4630.

I think an adequate fix would therefore be to basically roll this issue into #4630. That means:

Then in the future when we fix #4630, where it'll be easy to fix this issue at the same time, we'll naturally do so.

In the meantime, the user experience in this situation won't be any different from now, but it won't appear in our Sentry issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant