This type needs to agree with the route: RouteProp<…> on the component.

(This is one of the rare places where the type-checker isn't able to check this sort of thing for you, because of limitations in the types in the react-navigation library: #4757 (comment) .)

Component names are capitalized in our code, following normal React convention: search for "capital" in https://reactjs.org/docs/components-and-props.html .

The local name on this line, MoveMessage, is capitalized correctly. The module / filename should be capitalized the same way.

Make the word boundary visible: isAdmin, not isadmin (for both the local and the property).

id is too broad -- where it's used later in the code, it makes it hard to see what kind of thing it's identifying. Better would be messageId. Or just message, and say message.id where it's used.

"java"? I think this value isn't what you intended.

This also points out a need for better type-checking here, by writing down more specific types: we don't actually want api.updateMessage to allow just any string for the propagate_mode it gets passed. With better types there, Flow would be able to point out that this line is wrong.

To do that, make a preparatory commit (which just means a commit early in the branch, before the main commit) that edits src/api/messages/updateMessage.js to give a more precise type for propagate_mode. Refer to the server API documentation, at the page linked to in that function's jsdoc, for what the type should be.

Note that in Flow if you have a value that should be one of a fixed list of strings, like "either foo or bar", you can spell that type like 'foo' | 'bar'. See docs: https://flow.org/en/docs/types/unions/#toc-union-type-syntax

Some of these styles don't seem to be actually used in the code. Should delete them so it's easier to see the ones that do matter.

Variable names start with lower case. (Except if their value is a React component type, or a class (not just an instance of a class, but a class itself), both of which are rare.)

This comment appears to be copy-pasted from ComposeBox.js. Is the comment true here? What does it mean?

… Reading a bit more, it looks like in fact textInput will always be null here -- it comes from topicInputRef.current, and we never set topicInputRef or pass it somewhere that could end up setting it. So this function never does anything.

This appears to be setting properties on the function (which is a global constant for this module), in order to store some of the data that we're currently processing.

Do not do this. This is a brittle pattern -- effectively you're storing these values as global variables.

Instead, use the mechanisms that the surrounding code already uses to pass the data that comparable functions need. Here, moveMessage needs to be a Button<MessageArgs>, because that's what's in the array that constructMessageActionButtons returns. Looking at the definition of Button<Args>, in general the function takes an Args -- so for Button<MessageArgs> it takes a MessageArgs. So we just need the relevant information to be added to MessageArgs.

Specifically we need narrow there. For ownUser.is_admin, the moveMessage callback doesn't really need that information -- the MoveMessage component will need it, but it can get that information from Redux for itself.

BTW in general if you see a way to make the code work that involves structuring it with a bad pattern, and you can't find a cleaner way to do it, it's OK to send a PR where it uses the bad structure (especially when it's a small part of the PR's code, like here) -- but it's important to call attention to it clearly. Typically that might mean

• both a TODO comment in the code,
• and a prominent note about it in the PR description, or as a PR comment shortly after sending the PR.

That way you help the reviewer make sure to notice it so they can offer suggestions on how to handle the problem.

Better yet, typically, is to ask in chat about how to handle the thing you're trying to do, before you have the PR ready to send. But sometimes the easiest way to make the question clear is with the context of the rest of the code, and then a PR with clear communication about the question is a good way to go.

This will go and make a request to the server to look up what stream has the given name.

We don't want to do that. We already have data on all the streams the user has access to know about, and that's the data we used for presenting the UI the user will have used to select the stream. We should use that data instead.

That's better because not only does it make things faster, and more reliable on a weak network connection -- because we save a request round-trip -- but also it keeps the action we actually take aligned with the action the user chose, eliminating a class of bugs where there was a mismatch between the data we had at the moment the user chose something in the UI and the data the server has by the time it handles the request.

For example, if I go to move something to stream #awesome-team, and right around that moment someone else goes and renames #awesome-team to #awesome team, things should just work -- the messages should get moved to that stream, even though its name was #awesome team by the time the request made it to the server.

(This connects to the point above about using stream IDs rather than stream names.)

handlePropagateMode

So this function is to handle the propagate model while using the Picker component.

After I added the types for the propagate_mode in updateMessage API, the setPropagateMode function does not work with onValueChange function of the Picker component(line 202).

Since there will be only three values, I used if-else statements based on the values of the index

This works, but is fragile: these conditions in handlePropagateMode now have to line up with the order of options in the picker, and they're separated by quite a bit of code. It'd be easy for them to get out of sync, so that the items no longer had the effects they're labelled for.

For that reason it's better to use the value feature of Picker / PickerItem.

If you get errors from Flow but you're confident that what actually happens with the data is correct, you can add a $FlowFixMe comment with an explanation of why the actual behavior is correct.

handleNarrow

Noticed a bug when I made the PR. When move message function was accessed through the topic narrow, the topic auto complete didn't suggest any topic. So I had to create a new stream narrow.

To create a stream narrow, I needed the name of the stream.

First idea was to use the label I added to the items in the Picker component, but I later realized that I can't access the label after adding it, so I used the below method.

Since I had the id of the stream, mapped through the current stream list, and found the name,

Cool, looking up the stream name given the stream ID is the right direction.

Don't do it with a linear search through the whole list of streams, though: #3339.

Instead, use getStreamsById, similar to the getStreamsByName I mentioned in another comment.

Avoid linear searches like this one: #3339

That's why at #5189 (comment) I recommended getStreamsByName, which is a data structure for accomplishing the same lookup efficiently.

Don't use Promise#catch; instead, make the surrounding function async, and use await inside a try/catch block. (You can search for await api to find a number of examples.)

The reason is that with Promise#catch it's hard to avoid having subtle edge cases where exceptions don't get handled properly, and using try/catch blocks instead gives a much more robust structure for managing our exception handling.

(This isn't something that's obvious a priori. I wish I had a good writeup of it to link you to, but I don't.)

Oh, and here's a related benefit of using try/catch: you have these two .catch calls that are completely identical, and it would be better to say that just once. With try/catch, that's very straightforward to do: just put the try/catch outside the if/else that controls the two different paths.

Ah, here's an example of how Promise#catch tends to lead to confused error-handling, which is easier to see and avoid when using try/catch.

This code will unconditionally show a toast with "Moved Message", even if the move actually ends up failing.

Instead, we should show a confirmation message like that only after we actually succeed.

Both the error message and the success message should be translated. Search for showErrorAlert to see examples of how it's done at other call sites. See https://github.com/zulip/zulip-mobile/blob/main/docs/howto/translations.md#all-code-contributors-wiring-our-code-for-translations for background.

Write the confirmation message in sentence case: only the first word capitalized, so like "Moved message".

This works, but is fragile: these conditions in handlePropagateMode now have to line up with the order of options in the picker, and they're separated by quite a bit of code. It'd be easy for them to get out of sync, so that the items no longer had the effects they're labelled for.

For that reason it's better to use the value feature of Picker / PickerItem.

If you get errors from Flow but you're confident that what actually happens with the data is correct, you can add a $FlowFixMe comment with an explanation of why the actual behavior is correct.

These api.foo calls need await.

As discussed here:
#5189 (comment)
search for await api (e.g., use git grep -C2 "await api") to find examples.

Without that, the try/catch basically has no effect. You can demonstrate that by editing the implementation of api.updateMessage to artificially add an error, like throw new Error("oops"); or null.asdf;.

These would be clearer by unifying as one api.updateMessage call, because almost everything is the same in the two cases.

Instead, use a conditional just to control the one thing that changed. One handy trick for this, if you want an extra property in one case and not the other, is to spread a conditional expression:

  {
    subject,
    ...(isAdmin ? { stream_id: streamId } : {}),
    propagate_mode: propagateMode,
  }

Let's factor this expression out as a local variable in the function.

That way it gets a name that helps explain what its intended meaning is. It's kind of opaque as it is.

Hmmm, is the idea of this that we're taking the HTML of the message and attempting to turn it into plain text? I don't think this strategy is going to work for making something readable, in general, for complex messages.

Instead of attempting to turn the HTML into readable text, I think a more reliable strategy will be to use the message's Markdown content.

We can get that from the server using api.getRawMessageContent. See editMessage in src/action-sheets/index.js for an example.

Cool, this fixme comment is helpful and this code is clearer (cf #5189 (comment) on the previous version.)

Let's do two things to help clean this up further:

• Let's take some of the information that's in that comment, and move it into code: write a cast, like setPropagateMode((itemValue: 'change_one' | 'change_later' | 'change_all')).
• Flow will still give an error and require a fixme. But now the code itself already expresses the list of possible values we think it can be. So the fixme comment is now free to explain why -- namely, because those are the values in the items right below this.

nit: blank line between external and internal imports

Hmmm this doesn't get translated, does it.

Use ZulipTextIntl for this sort of UI text. If you search the codebase for <Text, you'll notice we have very few places using Text directly.

That will also ensure the text gets an appropriate style. In particular, looking back at the screenshots in dark mode I see that the contrast is pretty low -- the text is harder to read than it should be. ZulipTextIntl will get the text color from the theme, which means it'll get an appropriate color to contrast with the theme's background color.

Don't try to make this sort of custom replacement for common widgets that exist across the app. Instead, use the same standard widgets that the rest of the app uses.

You can see the consequences in your screenshots at the top of the thread. The "go back" arrow here doesn't match what appears in the rest of the app:

Similarly, having the screen's title in the middle of the app bar is inconsistent with the rest of the app, which has it at the left.

To find the right pattern to use, find any other screen in the app that has the piece of UI you're looking for -- so here, an app bar with a go-back button -- and look at the code for that screen.

nit: Organize this by grouping together different kinds of definitions, with blank lines to separate groups.

Follow the patterns seen in other components in the codebase: first props, then context, then selectors, then state.

These and some other spots would be made a bit simpler to read by pulling the route params out as locals:

  const { message, messageNarrow } = props.route.params;

That also replaces the need for a messageId local -- can just say message.id.

What's the intended meaning of messageNarrow? This could use some jsdoc here to clarify.

I'm not sure what it's supposed to mean here. That in turn is necessary in order to properly review the logic below that consumes it.