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

Composebox cleanup #4773

Merged
merged 12 commits into from
Aug 26, 2021
Merged
62 changes: 56 additions & 6 deletions src/chat/ChatScreen.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* @flow strict-local */
import React from 'react';
import React, { useCallback, useContext } from 'react';
import type { Node } from 'react';
import { useIsFocused } from '@react-navigation/native';

Expand All @@ -17,11 +17,17 @@ import InvalidNarrow from './InvalidNarrow';
import { fetchMessagesInNarrow } from '../message/fetchActions';
import ComposeBox from '../compose/ComposeBox';
import UnreadNotice from './UnreadNotice';
import { canSendToNarrow } from '../utils/narrow';
import { canSendToNarrow, caseNarrowDefault, keyFromNarrow } from '../utils/narrow';
import { getLoading, getSession } from '../directSelectors';
import { getFetchingForNarrow } from './fetchingSelectors';
import { getShownMessagesForNarrow, isNarrowValid as getIsNarrowValid } from './narrowsSelectors';
import { getFirstUnreadIdInNarrow } from '../message/messageSelectors';
import { getDraftForNarrow } from '../drafts/draftsSelectors';
Copy link
Contributor

@chrisbobbe chrisbobbe Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ComposeBox: Add defaultTopic and defaultMessage props.

This pulls out the editMessage/draft based topic setting, and moves that
responsibility to the caller, where it's easier to ensure that the code
is correct.

This also lets us finally get rid of UNSAFE_componentWillReceiveProps,
and replace it with a React key.

Hmm, could you say more about how adding defaultTopic and defaultMessage lets us get rid of UNSAFE_componentWillReceiveProps? What problems might we run into if we tried to get rid of it before adding those props? The changes look orthogonal to me, but I might be missing something. Or, if there would be problems, how about putting it after?

It'd be good to keep the commit that removes UNSAFE_componentWillReceiveProps as focused as possible. I do think the key approach is probably the right one, for reasons I've mentioned on CZO, but it's not a small change: the component will start getting completely unmounted and remounted, losing all of its state; that'll happen whenever key changes.

If it does introduce a bug, it'll probably be a puzzling one, the kind that might be easiest to find with a git bisect, and we'll want to help that investigation go smoothly. Also, I think it would be helpful for that commit to link to the discussion of the key solution, like https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Next.20steps.20for.20.60ComposeBox.60/near/1160876.

Copy link
Contributor

@chrisbobbe chrisbobbe Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to keep the commit that removes UNSAFE_componentWillReceiveProps as focused as possible.

Well, here's one behavior change I've just found, and it's a nice one! Just before this commit, I get the following (and also on master I get something that looks the same) when I start editing a message, along with lots of lag. It goes away at this commit; I suspect that's because of removing UNSAFE_componentWillReceiveProps. I read a bit about the error message and didn't come up with much (facebook/react#18178) and I decided not to look further; we were using something marked UNSAFE after all, so it seems like not-doing-that is a fine fix, seeing as it seems to work. 🤷

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, could you say more about how adding defaultTopic and defaultMessage lets us get rid of UNSAFE_componentWillReceiveProps? What problems might we run into if we tried to get rid of it before adding those props? The changes look orthogonal to me, but I might be missing something. Or, if there would be problems, how about putting it after?

You're correct, these are orthogonal. I've refactored the commits to avoid mixing them up.

import { addToOutbox } from '../actions';
import { getAuth, getCaughtUpForNarrow } from '../selectors';
import { showErrorAlert } from '../utils/info';
import { TranslationContext } from '../boot/TranslationProvider';
import * as api from '../api';

type Props = $ReadOnly<{|
navigation: AppNavigationProp<'chat'>,
Expand Down Expand Up @@ -103,10 +109,13 @@ export default function ChatScreen(props: Props): Node {
const { backgroundColor } = React.useContext(ThemeContext);

const { narrow, editMessage } = route.params;
const setEditMessage = (value: EditMessage | null) =>
navigation.setParams({ editMessage: value });
const setEditMessage = useCallback(
(value: EditMessage | null) => navigation.setParams({ editMessage: value }),
[navigation],
);

const isNarrowValid = useSelector(state => getIsNarrowValid(state, narrow));
const draft = useSelector(state => getDraftForNarrow(state, narrow));

const {
fetchError,
Expand All @@ -120,6 +129,43 @@ export default function ChatScreen(props: Props): Node {
const sayNoMessages = haveNoMessages && !isFetching;
const showComposeBox = canSendToNarrow(narrow) && !showMessagePlaceholders;

const auth = useSelector(getAuth);
const dispatch = useDispatch();
const caughtUp = useSelector(state => getCaughtUpForNarrow(state, narrow));
const _ = useContext(TranslationContext);

const sendCallback = useCallback(
(message: string, destinationNarrow: Narrow) => {
if (editMessage) {
const content = editMessage.content !== message ? message : undefined;
const subject = caseNarrowDefault(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change to start using caseNarrowDefault? I think it belongs in a different commit; this one is described as a simple "switch from A to B" commit.

Ah, looking again: possibly this was a small rebase error? 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I think I had this in there the whole time, and I agree that the commit doesn't describe that. The reason that it's needed is that the previous code could look directly at the state.topic, but the callback doesn't have access to the state.

I've added a commit to make that change in the original code.

destinationNarrow,
{ topic: (stream, topic) => (topic !== editMessage.topic ? topic : undefined) },
() => undefined,
);

if (
(content !== undefined && content !== '')
|| (subject !== undefined && subject !== '')
) {
api.updateMessage(auth, { content, subject }, editMessage.id).catch(error => {
showErrorAlert(_('Failed to edit message'), error.message);
});
}

setEditMessage(null);
} else {
if (!caughtUp.newer) {
showErrorAlert(_('Failed to send message'));
return;
}

dispatch(addToOutbox(destinationNarrow, message));
}
},
[_, auth, caughtUp.newer, dispatch, editMessage, setEditMessage],
);

return (
<KeyboardAvoider style={[componentStyles.screen, { backgroundColor }]} behavior="padding">
<ChatNavBar narrow={narrow} editMessage={editMessage} />
Expand Down Expand Up @@ -147,8 +193,12 @@ export default function ChatScreen(props: Props): Node {
{showComposeBox && (
<ComposeBox
narrow={narrow}
editMessage={editMessage}
completeEditMessage={() => setEditMessage(null)}
isEditing={editMessage !== null}
initialTopic={editMessage ? editMessage.topic : undefined}
initialMessage={editMessage ? editMessage.content : draft}
onSend={sendCallback}
autoFocusMessage={editMessage !== null}
Copy link
Contributor

@chrisbobbe chrisbobbe Jun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ChatScreen: Autofocus edited messages.

This fixes a regression introduced when switching how we handled edited
messages.

I think there might be some more details in your head that could helpfully be written down? 😄 I'm imagining coming to this in the future while trying to work out why the change was made (e.g., because it's relevant to some bug that we find), and feeling like there should be an answer but I'm not quite finding it.

Was the regression introduced earlier in the PR? If so, let's look for a way to not introduce it in the first place. If that's not going to work (e.g., because it forces us to obscure more than we clarify), then maybe a good workaround is to minimize the number of commits in which the regression is active, and fence it off clearly (the commit that introduces the regression and the commit that fixes it both mention each other and how far apart they are). I took this approach once, in the consecutive commits cc0126d and 3fa7a7f, where the regression was a small UI thing and I felt each change needed some breathing room to help it be understood.

If the regression is already in master, best would be to find the commit ID and reference that; otherwise, maybe a PR number?

Then, I'm interested in knowing what the regression was and why this is the right fix for it. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding was that this was a regression when switching to use key, although I'm not 100% sure why/how. I've refactored the commits so that this line is added in the commit that switches to using key in the first place, so as not to introduce the problem.

Copy link
Contributor

@chrisbobbe chrisbobbe Jun 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding was that this was a regression when switching to use key, although I'm not 100% sure why/how. I've refactored the commits so that this line is added in the commit that switches to using key in the first place, so as not to introduce the problem.

Cool. It seems very plausible that the regression, whatever it was, would have been introduced in the key commit. If that's what you observed empirically, I think "it has something to do with lost state because of unmounting/remounting a ComposeBox when its key changes (not merely updating/rerendering)" is a rough sketch at an explanation for the change, at a good-enough level of detail, which let's put into the commit message (with maybe better wording). Could also mention a description of the wrong behavior observed in the regression.

Also let's link to the CZO discussion where I mentioned the key idea, and possibly to the documentation I drew from, so our choice of strategy for removing UNSAFE_componentWillReceiveProps is clear and inspectable in the future. 🙂

Elsewhere in this review, I floated the idea of lumping into this commit the change to have state.topic and state.message's initial values depend on the edit-message state. If we're doing that, I think the same reason applies, except in that case we know precisely what bits of state are lost and need to be re-initialized with the right values: state.topic and state.message.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also mention the 100+ appearances of an error, in a loop, that seem to get resolved in this commit 🎉: #4773 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, glad to start having autoFocusMessage, initialTopic, and initialMessage depend on editMessage in this commit. In the commit message, let's write down our reason for making those changes in this commit, as discussed above (4691ff2#r648445186), so that it's clear and inspectable in the future. 🙂

Also, as I look again, I'm glad to have the mention of the "Warning: Cannot update a component from inside…" fix in the commit message, since this sure seems to be the commit that changes that behavior for the better. But while it's nice (and potentially really helpful for responding to bug reports) to know that the specific behavior is improved, it's also true that we didn't end up with a really precise diagnosis, and I think we should mention that. As-is, I think it's possible for someone to follow the link after "See the CZO thread" in hopes of finding a diagnosis, and they'll be disappointed and possibly confused when they don't find one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment about this in the commit message. I don't think it's as mysterious as our initial conversation made it out to be — there was some logic in UNSAFE_componentWillReceiveProps, and those props are a way to replicate that in a non-imperative way, but it still seems worth mentioning. Also updated the bit about the error message disappearing.

Copy link
Contributor

@chrisbobbe chrisbobbe Jul 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's as mysterious as our initial conversation made it out to be

Understood. 🙂 But we depend heavily on reading a clear commit history, and I think I've mentioned elsewhere that the key change is somewhat likely to introduce puzzling regressions, almost no matter how carefully we do it. So it'll be nice to have these details at hand if that happens, and not have to guess as much about why we made certain changes and whether the reasons were good based on what we knew at the time.

Also, I think explaining these prop changes does more than just answer why we made them: it clarifies a bit the limits of what we considered. Without these explanations, I think someone in the future is more likely to assume that we deduced that these changes were all we had to do toward replicating the logic in UNSAFE_componentWillReceiveProps in a non-imperative way. We didn't deduce that; we started with some prominent behaviors that we wanted to keep, without considering some small-looking things at all, like a textInput.setNativeProps call (whatever that's about) and everything we do with TextInputReset. I think that's probably an OK tradeoff, so that we don't spend too long on this. But it is a tradeoff, and I think it'll be more obvious that we made the tradeoff after listing these things that we did do something about.

key={keyFromNarrow(narrow) + (editMessage?.id.toString() ?? 'noedit')}
/>
)}
</KeyboardAvoider>
Expand Down
126 changes: 47 additions & 79 deletions src/compose/ComposeBox.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,9 @@ import { ThemeContext } from '../styles';
import type {
Auth,
Narrow,
EditMessage,
InputSelection,
UserOrBot,
Dispatch,
CaughtUp,
GetText,
Subscription,
Stream,
Expand All @@ -28,39 +26,33 @@ import type {
} from '../types';
import { connect } from '../react-redux';
import { withGetText } from '../boot/TranslationProvider';
import { addToOutbox, draftUpdate, sendTypingStart, sendTypingStop } from '../actions';
import * as api from '../api';
import { draftUpdate, sendTypingStart, sendTypingStop } from '../actions';
import { FloatingActionButton, Input } from '../common';
import { showErrorAlert, showToast } from '../utils/info';
import { showToast } from '../utils/info';
import { IconDone, IconSend } from '../common/Icons';
import {
isStreamNarrow,
isStreamOrTopicNarrow,
isTopicNarrow,
streamNameOfNarrow,
topicNarrow,
topicOfNarrow,
} from '../utils/narrow';
import ComposeMenu from './ComposeMenu';
import getComposeInputPlaceholder from './getComposeInputPlaceholder';
import NotSubscribed from '../message/NotSubscribed';
import AnnouncementOnly from '../message/AnnouncementOnly';
import MentionWarnings from './MentionWarnings';

import {
getAuth,
getIsAdmin,
getLastMessageTopic,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should follow up by having the "Topic" input be the default thing
that's focused, rather than the "Message" input.

This seems like it'd be pretty easy to do, with the autoFocusTopic prop you introduce later in the PR? 😅

Copy link
Contributor Author

@WesleyAC WesleyAC Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly not — this is referring to when you go and tap on the "Message" input box, it should actually focus the topic input box, but we don't want to autoFocus it in the sense that it'll grab your keyboard as soon as you load the component.

getCaughtUpForNarrow,
getStreamInNarrow,
getVideoChatProvider,
} from '../selectors';
import { getAuth, getIsAdmin, getStreamInNarrow, getVideoChatProvider } from '../selectors';
import {
getIsActiveStreamSubscribed,
getIsActiveStreamAnnouncementOnly,
} from '../subscriptions/subscriptionSelectors';
import { getDraftForNarrow } from '../drafts/draftsSelectors';
import TopicAutocomplete from '../autocomplete/TopicAutocomplete';
import AutocompleteView from '../autocomplete/AutocompleteView';
import { getAllUsersById, getOwnUserId } from '../users/userSelectors';
import * as api from '../api';

type SelectorProps = {|
auth: Auth,
Expand All @@ -69,21 +61,35 @@ type SelectorProps = {|
isAdmin: boolean,
isAnnouncementOnly: boolean,
isSubscribed: boolean,
draft: string,
lastMessageTopic: string,
caughtUp: CaughtUp,
videoChatProvider: VideoChatProvider | null,
stream: Subscription | {| ...Stream, in_home_view: boolean |},
|};

type OuterProps = $ReadOnly<{|
narrow: Narrow,
editMessage: EditMessage | null,
completeEditMessage: () => void,

onSend: (string, Narrow) => void,

isEditing: boolean,

/** The contents of the message that the ComposeBox should contain when it's first rendered */
initialMessage?: string,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The component so far hasn't set a good example of this, of course, but: let's add some jsdoc for at least the props we add, to be explicit about ComposeBox's interface.

That'll be a clear sign facing callers (well, the one caller) so they can be sure to use ComposeBox correctly, without having to click through to the implementation.

It'll also be a clear sign facing inward, toward the implementation, so it can develop without breaking any expectations that the callers (caller) might have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :)

/** The topic of the message that the ComposeBox should contain when it's first rendered */
initialTopic?: string,

/** Whether the topic input box should auto-foucs when the component renders.
*
* Passed through to the TextInput's autofocus prop. */
autoFocusTopic?: boolean,
/** Whether the message input box should auto-foucs when the component renders.
*
* Passed through to the TextInput's autofocus prop. */
autoFocusMessage?: boolean,
|}>;

type Props = $ReadOnly<{|
...OuterProps,
...SelectorProps,

// From 'withGetText'
_: GetText,
Expand Down Expand Up @@ -155,8 +161,10 @@ class ComposeBoxInner extends PureComponent<Props, State> {
isFocused: false,
isMenuExpanded: false,
height: 20,
topic: this.props.lastMessageTopic,
message: this.props.draft,
topic:
this.props.initialTopic
?? (isTopicNarrow(this.props.narrow) ? topicOfNarrow(this.props.narrow) : ''),
message: this.props.initialMessage ?? '',
Comment on lines +164 to +167
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This diff is doing a couple of unrelated things in addition to the point of the commit:

ComposeBox: Don't set topic to last message topic.

This is typically confusing to users, so just remove it and force people
to either specify the topic or send to "(no topic)".

In particular it's switching the message logic from || to ??, which isn't obviously equivalent, as well as the topic logic where it is.

It'd be cleanest to have the earlier commit that introduces this logic use ?? in the first place. Then this one wouldn't touch message, and would touch topic only to change what the default is.

selection: { start: 0, end: 0 },
numUploading: 0,
};
Expand All @@ -174,8 +182,8 @@ class ComposeBoxInner extends PureComponent<Props, State> {
};

getCanSelectTopic = () => {
const { editMessage, narrow } = this.props;
if (editMessage) {
const { isEditing, narrow } = this.props;
if (isEditing) {
return isStreamOrTopicNarrow(narrow);
}
if (!isStreamNarrow(narrow)) {
Expand Down Expand Up @@ -291,9 +299,11 @@ class ComposeBoxInner extends PureComponent<Props, State> {

handleMessageChange = (message: string) => {
this.setState({ message, isMenuExpanded: false });
const { dispatch, narrow } = this.props;
const { dispatch, isEditing, narrow } = this.props;
dispatch(sendTypingStart(narrow));
dispatch(draftUpdate(narrow, message));
if (!isEditing) {
dispatch(draftUpdate(narrow, message));
}
};

// See JSDoc on 'onAutocomplete' in 'AutocompleteView.js'.
Expand All @@ -320,13 +330,11 @@ class ComposeBoxInner extends PureComponent<Props, State> {
};

handleMessageFocus = () => {
this.setState((state, { lastMessageTopic }) => ({
...state,
topic: state.topic || lastMessageTopic,
this.setState({
isMessageFocused: true,
isFocused: true,
isMenuExpanded: false,
}));
});
};

handleMessageBlur = () => {
Expand Down Expand Up @@ -362,8 +370,8 @@ class ComposeBoxInner extends PureComponent<Props, State> {
};

getDestinationNarrow = (): Narrow => {
const { narrow } = this.props;
if (isStreamNarrow(narrow)) {
const { narrow, isEditing } = this.props;
if (isStreamNarrow(narrow) || (isTopicNarrow(narrow) && isEditing)) {
const streamName = streamNameOfNarrow(narrow);
const topic = this.state.topic.trim();
return topicNarrow(streamName, topic || '(no topic)');
Expand All @@ -372,15 +380,11 @@ class ComposeBoxInner extends PureComponent<Props, State> {
};

handleSend = () => {
const { dispatch, narrow, caughtUp, _ } = this.props;
const { dispatch } = this.props;
const { message } = this.state;
const narrow = this.getDestinationNarrow();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ComposeBox: Send "stop typing" notification to destination narrow.

This sends the "stop typing" notification to the destination narrow,
rather than the narrow that the ComposeBox happened to be loaded into.

This isn't totally correct - we may still miss sending some stop typing
notifications that we should send, if a user makes edits to the message
and then later edits the topic, but this change is still an improvement.

This change is good, but the commit message should mention that all of this is in the future when we start sending typing notifications for stream messages at all; i.e., this is a latent bug, lying in wait for when we add that feature.

Otherwise it's confusing for someone who knows that we only send typing notifications for PMs, as they try to figure out what scenario the commit message is talking about in which we're sending typing notifications and then the destination narrow changes.

Hmm, in fact: I think even in that future, this won't at least primarily be about the destination narrow changing. The typical case where this.getDestinationNarrow() is different from this.props.narrow -- the only case outside of message-editing scenarios -- is when you're in a stream narrow, in which case the destination narrow is always some topic narrow, so always different from the stream narrow. So the latent bug this is fixing is that when we go to add typing notifications for stream messages, when you're in a stream narrow we'd try to send these stopped-typing notifications to the stream narrow, instead of the specific topic. Probably that would look like not sending them at all -- probably sendTypingStop would ignore them, just like it currently does everything but PM narrows.


… Hmm and actually, looking at the implementation of getDestinationNarrow, a this.props.narrow of a stream narrow is the only case where it returns something different from the prop. That seems like a bug -- if you're in a topic narrow, start editing a message, and edit the topic input, the right answer should use the value from the topic input.

Does that affect any of the other changes in this branch? I feel like there were some refactorings that relied on getDestinationNarrow, and may have turned a latent bug (that getDestinationNarrow returns a wrong value) into a live one (by using that value in a case where we didn't before.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the getDestinationNarrow bug in a new commit, but I think that may actually break editing messages. Looking into this more...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I was seeing #4830, which reproduces on master :/

I believe that the code as it is now is no more incorrect than it was previously, but I obviously could be missing something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That fix looks good, thanks! (Modulo the thread at #4773 (comment) .)


if (!caughtUp.newer) {
showErrorAlert(_('Failed to send message'));
return;
}
Comment on lines -378 to -381
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this reintroduce #3800? If not, why not?

In any case I don't think this benefits from being rolled into a long cleanup series like this one -- it's entirely about a behavior change, not anything to do with cleaning up the code. Better to have its own PR thread, to avoid adding complexity to this one.

Copy link
Contributor Author

@WesleyAC WesleyAC Jun 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does — I hadn't realized this history of this. I had thought that something about this PR made the symptoms described in #mobile > Failed to send on Android worse, but looking at it again, I don't think that's the case. I've removed this commit.


dispatch(addToOutbox(this.getDestinationNarrow(), message));
this.props.onSend(message, narrow);

this.setMessageInputValue('');

Expand All @@ -391,41 +395,6 @@ class ComposeBoxInner extends PureComponent<Props, State> {
dispatch(sendTypingStop(narrow));
};

handleEdit = () => {
const { auth, editMessage, completeEditMessage, _ } = this.props;
if (!editMessage) {
throw new Error('expected editMessage');
}
const { message, topic } = this.state;
const content = editMessage.content !== message ? message : undefined;
const subject = topic !== editMessage.topic ? topic : undefined;
if ((content !== undefined && content !== '') || (subject !== undefined && subject !== '')) {
api.updateMessage(auth, { content, subject }, editMessage.id).catch(error => {
showErrorAlert(_('Failed to edit message'), error.message);
});
}
completeEditMessage();
if (this.messageInputRef.current !== null) {
// `.current` is not type-checked; see definition.
this.messageInputRef.current.blur();
}
};

UNSAFE_componentWillReceiveProps(nextProps: Props) {
Copy link
Contributor

@chrisbobbe chrisbobbe Jun 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ComposeBox: Stop using UNSAFE_componentWillReceiveProps.

This replaces UNSAFE_componentWillReceiveProps with a key prop, so that
we rerender the ComposeBox when we look at a different narrow or
start/stop editing a message.
[...]

This isn't correct: it doesn't cause the ComposeBox to rerender in the normal sense (that render is called, and any componentDidUpdate is called, etc.). The crucial thing that's happening, and that makes this strategy work, is that the old instance gets completely torn down and a new one is created. For example, we have this code that sets the initial state when a ComposeBox instance is constructed:

  state = {
    isMessageFocused: false,
    isTopicFocused: false,
    isFocused: false,
    isMenuExpanded: false,
    topic:
      this.props.initialTopic !== undefined
        ? this.props.initialTopic
        : isTopicNarrow(this.props.narrow)
        ? topicOfNarrow(this.props.narrow)
        : '',
    message: this.props.initialMessage || '',
    selection: { start: 0, end: 0 },
  };

That's how state.topic and state.message end up being set to the desired values when the key changes. It's this "instance field" bit, which I think of as a shortcut to setting things in the constructor. It only runs once during the lifecycle of an instance.

Copy link
Contributor

@chrisbobbe chrisbobbe Jun 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the key isn't quite correctly called a prop; I think I usually call it the "key attribute". You can't access it on this.props, and of course it has its special role that props don't have (https://reactjs.org/docs/lists-and-keys.html, https://reactjs.org/docs/reconciliation.html#keys).

Copy link
Contributor

@chrisbobbe chrisbobbe Jun 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a nice explanation of the strategy we've chosen, which I linked to in the CZO discussion (see the heading "Recommendation: Fully uncontrolled component with a key").

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! Reworded the commit.

if (nextProps.editMessage !== this.props.editMessage) {
const topic = nextProps.editMessage
? nextProps.editMessage.topic
: nextProps.lastMessageTopic;
const message = nextProps.editMessage ? nextProps.editMessage.content : '';
this.setMessageInputValue(message);
this.setTopicInputValue(topic);
if (this.messageInputRef.current !== null) {
// `.current` is not type-checked; see definition.
this.messageInputRef.current.focus();
}
}
}
Comment on lines -414 to -427
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎊 Glad to be getting rid of this code 😄 -- not only is it using an API labeled "UNSAFE", but the logic inside it is kind of tangly and dubious.


inputMarginPadding = {
paddingHorizontal: 8,
paddingVertical: Platform.select({
Expand Down Expand Up @@ -477,7 +446,7 @@ class ComposeBoxInner extends PureComponent<Props, State> {
ownUserId,
narrow,
allUsersById,
editMessage,
isEditing,
insets,
isAdmin,
isAnnouncementOnly,
Expand Down Expand Up @@ -533,6 +502,7 @@ class ComposeBoxInner extends PureComponent<Props, State> {
underlineColorAndroid="transparent"
placeholder="Topic"
defaultValue={topic}
autoFocus={this.props.autoFocusTopic}
selectTextOnFocus
textInputRef={this.topicInputRef}
onChangeText={this.handleTopicChange}
Expand All @@ -550,6 +520,7 @@ class ComposeBoxInner extends PureComponent<Props, State> {
underlineColorAndroid="transparent"
placeholder={placeholder}
defaultValue={message}
autoFocus={this.props.autoFocusMessage}
textInputRef={this.messageInputRef}
onBlur={this.handleMessageBlur}
onChangeText={this.handleMessageChange}
Expand All @@ -561,10 +532,10 @@ class ComposeBoxInner extends PureComponent<Props, State> {
<FloatingActionButton
accessibilityLabel="Send message"
style={this.styles.composeSendButton}
Icon={editMessage === null ? IconSend : IconDone}
Icon={isEditing ? IconDone : IconSend}
size={32}
disabled={message.trim().length === 0 || this.state.numUploading > 0}
onPress={editMessage === null ? this.handleSend : this.handleEdit}
onPress={this.handleSend}
/>
</View>
</View>
Expand All @@ -580,9 +551,6 @@ const ComposeBox: ComponentType<OuterProps> = compose(
isAdmin: getIsAdmin(state),
isAnnouncementOnly: getIsActiveStreamAnnouncementOnly(state, props.narrow),
isSubscribed: getIsActiveStreamSubscribed(state, props.narrow),
draft: getDraftForNarrow(state, props.narrow),
lastMessageTopic: getLastMessageTopic(state, props.narrow),
caughtUp: getCaughtUpForNarrow(state, props.narrow),
stream: getStreamInNarrow(state, props.narrow),
videoChatProvider: getVideoChatProvider(state),
})),
Expand Down
Loading