zulip/zulip-mobile

Stop using bogus data on unknown-stream ChatScreen

Open

#5,212 opened on Jan 29, 2022

View on GitHub
 (0 comments) (0 reactions) (0 assignees)JavaScript (673 forks)github user discovery
help wanted

Repository metrics

Stars
 (1,348 stars)
PR merge metrics
 (PR metrics pending)

Description

As we saw in #5206, it's possible to have a message in the message list that's in a stream we don't have in our data structures. (For example: if you were in a stream, got some messages there, then left it, and either it's private (and you're not an admin) or you're a guest. Then find that message in all-messages, in search, or if applicable in @-mentions or starred messages.) If you tap that recipient header, we navigate to ChatScreen as usual, where we'd show the message list; but instead of an actual message list, we try to show a nice error screen.

This issue is about a pair of buggy aspects of that screen:

  • There's a "Subscribe" button at the bottom, but you can't actually subscribe.
  • The stream icon is "muted", which doesn't make a lot of sense.

The root cause of both of these is NULL_SUBSCRIPTION, and specifically its use by getStreamInNarrow. In general NULL_SUBSCRIPTION is always a sign of dubious code, because it consists entirely of bogus made-up data. (This is one of the few remaining uses of it; we've already eliminated all its former siblings.) See the comment by its definition, and the discussion at 25125db94 and the other commits mentioned in that comment.

Here, what's happening is:

  • NotSubscribed calls getStreamInNarrow to supply a local stream, and later consults both stream.name and stream.invite_only. The bogus value that NULL_SUBSCRIPTION arbitrarily chooses for invite_only is false. So NotSubscribed behaves as if this stream is a public stream, and shows a "Subscribe" button.

    In reality, if the stream is unknown, then it's one our user cannot just subscribe to, and so NotSubscribed should not show that button.

  • TitleStream calls getStreamInNarrow, and later consults stream.stream_id, stream.name, stream.in_home_view, and stream.invite_only. The bogus value in NULL_SUBSCRIPTION for in_home_view is false. This causes StreamIcon to be passed isMuted: true, which causes it to show a "muted" icon.

    Instead, if the stream is unknown, we should show a more neutral "stream"/"hash" icon; or possibly the "private"/"lock" icon.

To fix this:

  • getStreamInNarrow on an unknown stream should return undefined, not NULL_SUBSCRIPTION.
  • Each caller should then handle that possibility appropriately. In NotSubscribed and TitleStream, use ?.. For example, stream?.invite_only ?? true.
  • Where we use stream.name, that can be stream?.name ?? '', preserving the current behavior. Fixing that will be a larger task, and a separate issue under the #5210 umbrella.

(This is part of the umbrella issue #5210.)

Contributor guide