zulip/zulip-mobile

Stop using bogus data on unknown-stream ChatScreen

Open

#5,212 建立於 2022年1月29日

在 GitHub 查看
 (0 留言) (0 反應) (0 負責人)JavaScript (673 fork)github user discovery
help wanted

倉庫指標

Star
 (1,348 star)
PR 合併指標
 (PR 指標待抓取)

描述

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.)

貢獻者指南