zulip/zulip-mobile

Give MentionWarnings a less fragile interface

Open

#5,058 opened on Oct 19, 2021

View on GitHub
 (1 comment) (0 reactions) (0 assignees)JavaScript (673 forks)github user discovery
a-compose/sendhelp wanted

Repository metrics

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

Description

This issue is to follow up on #4745 with the refactorings mentioned there, as described originally in chat.

First, consolidate the two props narrow and stream into just narrow:

But it seems like there's also a deeper issue, which is that this code is doing its work using stream but it's relying on narrow to tell it whether, basically, the data in stream is meaningful.

That sets us up for this kind of bug. Instead, […] probably actually the right thing is that MentionWarnings should take just a Narrow. After all, that "stream" value is just derived straight from narrow anyway, by the caller.

Then also, that prop should be called instead like destinationNarrow:

Along with removing the stream prop, I think MentionWarnings could reasonably grow an invariant that the "reply narrow" or "destination narrow"—the answer to the question, "where should replies to this message go?"—is a stream narrow or a topic narrow. Its caller could conditionally not call it, otherwise, and it could do a check, e.g., in its constructor. […]

Yeah, agreed -- the narrow that MentionWarnings is passed should be the destination narrow, not the narrow you're viewing. And then its name should reflect that explicitly.

We already do that for ComposeMenu:

          <ComposeMenu
            destinationNarrow={this.getDestinationNarrow()}

so we can easily start passing the same thing to MentionWarnings.

Contributor guide