Composebox cleanup #4773

WesleyAC · 2021-06-02T17:04:21Z

WesleyAC · 2021-06-02T17:08:37Z

The first two commits here a kinda big and messy, sorry about that :( but the rest are reasonably small and nice.

chrisbobbe

Thanks, @WesleyAC! I'm excited for ComposeBox to get detangled! Comments below.

chrisbobbe · 2021-06-03T01:42:48Z

src/chat/ChatScreen.js

@@ -188,6 +188,7 @@ export default function ChatScreen(props: Props) {
          onSend={sendCallback}
          defaultTopic={editMessage ? editMessage.topic : undefined}
          defaultMessage={editMessage ? editMessage.content : draft}
+          autoFocusMessage={editMessage !== null}


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

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.

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.

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

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.

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.

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.

chrisbobbe · 2021-06-03T21:04:02Z

src/compose/ComposeBox.js


-    if (this.mentionWarnings.current) {
-      this.mentionWarnings.current.clearMentionWarnings();
+      dispatch(sendTypingStop(narrow));


It looks like this commit changes the narrow we call sendTypingStop with, from props.narrow to this.getDestinationNarrow(). Could you make a small commit before this that makes just that non-pure-refactor change?

I think we do want to use the "destination narrow", the relevant PM or topic narrow for the message being composed. It makes sense for the typing state to be for that narrow, since that's where other users will normally be awaiting the message.

I think there's more to be done in that direction, though not necessarily in this PR. Before sending, it looks like we don't send typing-start and typing-stop notifications for the destination narrow, for each value that the destination-narrow state takes while the user is composing.

I think a good direction to go (could be in or outside this PR) might be to store the destination narrow in a piece of React state, instead of computing it from state.topic whenever it's needed. (In fact, doing that can probably make state.topic obsolete and let us sweep that away—we can think of the topic input as a widget whose only purpose is to change the destination narrow, right?)

This will make it easy to fire typing-start and typing-stop for each value that the destination narrow takes, with a simple componentDidUpdate (or useEffect when that day comes).

A more powerful compose box could also change the destination narrow of a PM message by changing the recipients, as the web app allows when composing a new PM. In that world, it may be convenient to extract the topic input into its own component, and add a component for PM-recipient input. Then pass each of those components a callback they'll use to tell ComposeBox to change the destination narrow. And ComposeBox itself doesn't have to worry about storing the topic or PM recipients in its own state.

Oh, also: looks like this commit starts having us use the destination narrow for the "Failed to send message" check, which is relevant to a known bug:

Hmm, I'm getting a "failed to send message" popup when trying to message in #test here , and I'm not sure why.

[…]

yeah, it bases it on whether you have any unfetched messages in the narrow you're in, not the topic you're trying to send to. that's just a bug no matter how you slice it.

So that's something else that would be great to stand on its own, and link to that conversation (I'm not finding a corresponding issue). 🙂 Hopefully extracting that out will leave this as a commit we can mark NFC (good for reviewing and history-reading), or not too many steps off from being NFC.

Added a commit to just do the const narrow = this.getDestinationNarrow() change, and added a link to that CZO discussion in the commit.

Removing state.topic as you describe seems like a good plan to me.

chrisbobbe · 2021-06-03T23:51:01Z

src/chat/ChatScreen.js

+          onSend={sendCallback}
+          defaultTopic={editMessage ? editMessage.topic : undefined}
+          defaultMessage={editMessage ? editMessage.content : draft}
+          key={JSON.stringify([editMessage, narrow])}


nit: Two Narrow objects we'd otherwise treat as equivalent could be JSON-stringified differently, depending on the order in which their properties were defined. keyFromNarrow(narrow) is the string designed to be a unique key for narrows.

chrisbobbe · 2021-06-04T00:23:07Z

src/chat/ChatScreen.js

@@ -23,6 +23,7 @@ import { getFetchingForNarrow } from './fetchingSelectors';
 import { getShownMessagesForNarrow, isNarrowValid as getIsNarrowValid } from './narrowsSelectors';
 import { addToOutbox } from '../actions';
 import { getAuth, getCaughtUpForNarrow } from '../selectors';
+import { getDraftForNarrow } from '../drafts/draftsSelectors';


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.

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

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.

chrisbobbe · 2021-06-04T00:30:34Z

src/chat/ChatScreen.js

+    dispatch: Dispatch,
+    _: GetText,
+    message: string,
+    callbackNarrow: Narrow,


Would it make sense to name this destinationNarrow? I think that matches its purpose pretty well, and I don't see other possible purposes; I'm looking at getDestinationNarrow in ComposeBox.

chrisbobbe · 2021-06-04T02:11:42Z

src/compose/ComposeBox.js

@@ -45,7 +45,6 @@ import MentionWarnings from './MentionWarnings';
 import {
  getAuth,
  getIsAdmin,
-  getLastMessageTopic,


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? 😅

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.

chrisbobbe · 2021-06-04T02:12:54Z

src/compose/ComposeBox.js

@@ -63,7 +62,7 @@ type Props = $ReadOnly<{|
  insets: EdgeInsets,

  narrow: Narrow,
-  editMessage: EditMessage | null,
+  editing: boolean,


nit: I might name this isEditing, for more explicitness that it's a boolean

chrisbobbe · 2021-06-04T02:24:32Z

src/compose/ComposeBox.js

+  autoFocusTopic: boolean,
+  autoFocusMessage: boolean,
+  useTopicAutocomplete: boolean,
+  alwaysShowTopic: boolean,


ComposeBox [nfc]: Add autoFocus props.

useTopicAutocomplete and alwaysShowTopic don't quite fit the description in the commit message.

Also, do you have a specific use for them in mind? It looks like we're not passing either one of them, in ComposeBox's only callsite.

One thing I'd caution about is that there are cases where it'll be inappropriate to show anything about a topic: when editing a PM, and when composing a new PM to send.

Instead of these two props, I wonder if there could be just one prop that says whether the compose box should be aimed at working on a PM message or a stream message.

Maybe most of getCanSelectTopic (all but the isFocused part, I think) can be pulled out of ComposeBox to get the value for that prop? That seems like a refactoring commit that might go in the right direction. (Separately, though, I don't like the way we use props.narrow to decide whether the compose box should be aware of topics—eventually, I'd like to stop refusing to show the compose box in narrows like the all-messages narrow, from which reasonable users will want to edit a message without having to find it in a different narrow first. That'll mean inspecting the actual message the user wants to edit, in some appropriate place, to see if it's a stream message or a PM, and have the ComposeBox show the right thing for each type.)

useTopicAutocomplete and alwaysShowTopic don't quite fit the description in the commit message.

Ugh, these accidentally got mixed in when I was rebasing, I used them later in the new topic UI work. They're gone in my new revision.

I don't think that the refactor you describe would work for the new topic UI, although it seems like a reasonable idea in general.

The idea behind these was:

useTopicAutocomplete: When you're making a new topic, we don't want to autocomplete old topic names.
alwaysShowTopic: This makes it show the "Topic" input box even when the composebox isn't focused, which is desirable for the new topic UI, but not for general use. What we probably actually want is a showTopicBox: 'always' | 'focus' | 'never'; with focus as the default, or something like that.

Got it, makes sense.

I wonder if there could be just one prop that says whether the compose box should be aimed at working on a PM message or a stream message.

I think exploring a prop like the one I've mentioned sounds like work for a different PR. I'm interested in looking into that at some point, if it sounds OK to you; I'm not aware that there would be many obstacles to adding it, but you're right that it wouldn't get us any farther toward making the new-topic UI.

For the new-topic UI, instead of useTopicAutocomplete and alwaysShowTopic, what do you think about a single prop like isRequestingNewTopic (or some better name)? If that sounds OK to you, I think it could be a good anchor for that part of ComposeBox's interface, to help keep it steady until ComposeBox (eventually) has better support for PMs. When that time comes, we'll naturally be curious about adding the analogous "new PM" UI that the web app has alongside "new topic"—

—and I could imagine it being nice to add a simple isRequestingNewPm prop alongside isRequestingNewTopic (or perhaps coalescing the two into isRequestingNewDestinationNarrow if that turns out to be worth considering).

My reason for doing separate props was so that it would be clearer what they actually do, but I see the argument for bundling them as well.

Perhaps we should have a mode: 'stream' | 'topic' | 'pm' | 'newTopic' | 'newPm'; or something like that? We wouldn't want to let someone set both isRequestingNewPm and isRequestingNewTopic.

My reason for doing separate props was so that it would be clearer what they actually do

Good to make it clear what things do, although often a good jsdoc can do that job well (and we're trying to be better about that than the current state of ComposeBox) 🙂.

Perhaps we should have a mode: 'stream' | 'topic' | 'pm' | 'newTopic' | 'newPm'; or something like that? We wouldn't want to let someone set both isRequestingNewPm and isRequestingNewTopic.

Seems worth a try; I like that this means we don't have to think about the possibility of contradictory settings being set.

WesleyAC · 2021-06-04T22:39:56Z

I pushed a bunch of revisions that make the commit history nicer here, and fix some of the comments you brought up. Will reply inline.

WesleyAC · 2021-06-04T22:50:31Z

Should be ready for another review @chrisbobbe :)

chrisbobbe

Thanks for the revision! Comments below.

chrisbobbe · 2021-06-09T00:52:23Z

src/chat/ChatScreen.js

+    destinationNarrow: Narrow,
+  ) => {
+    const state = getState();
+    const caughtUp = getCaughtUpForNarrow(state, destinationNarrow);


Bump on #4773 (comment) 🙂:

Oh, also: looks like this commit starts having us use the destination narrow for the "Failed to send message" check […]

[…]

So that's something else that would be great to stand on its own […]. 🙂 Hopefully extracting that out will leave this as a commit we can mark NFC (good for reviewing and history-reading), or not too many steps off from being NFC.

The commit I'm thinking would be good to be marked NFC is the ComposeBox: Pull sending logic out of ComposeBox. one, since it includes a pure refactor. First, we just have to isolate the pure refactor there. If the non-refactor change(s) happen to cause or reveal bugs, we'll want to look back at the history without the distraction of a pure refactor in the same commit.

chrisbobbe · 2021-06-09T02:09:42Z

src/compose/ComposeBox.js

-      const topic = nextProps.editMessage
-        ? nextProps.editMessage.topic
-        : nextProps.lastMessageTopic;
+      const topic = nextProps.editMessage ? nextProps.editMessage.topic : '';


ComposeBox: Don't set topic by default in stream narrows. This is typically confusing to users, so just remove it and force people to specify the topic they want. We should follow up by having the "Topic" input be the default thing that's focused, rather than the "Message" input. Fixes: #4764

Re: the summary line, I think the headline change we want to make is to stop resetting the topic in most kinds of narrows where stream messages might appear, not just stream narrows. For example, I could imagine having an experience like #4764 except in the all-messages (home) narrow. Does that sound right?

Also (and this affects the code, too) I think we could consider an exception: topic narrows. From reading the code, it looks like state.topic will be left as the empty string if you're in a topic narrow, you've edited a message, and you're not currently editing a message. I don't think this currently has a user-visible effect, because when props.narrow is a topic narrow this.getDestinationNarrow() just returns that topic narrow, but it seems right a priori to not leave this gap in the correctness of state.topic in topic narrows.

How about something like this?

const topic = nextProps.editMessage ? nextProps.editMessage.topic : isTopicNarrow(nextProps.narrow) ? topicOfNarrow(nextProps.narrow) : '';

Ah, and it also looks like this commit has another effect we might not want in topic narrows: in the constructor, it has us not initialize state.topic with the topic-narrow topic.

chrisbobbe · 2021-06-09T02:27:38Z

src/chat/ChatScreen.js

@@ -139,6 +141,8 @@ export default function ChatScreen(props: Props) {
          narrow={narrow}
          editMessage={editMessage}
          completeEditMessage={() => setEditMessage(null)}
+          defaultTopic={editMessage ? editMessage.topic : undefined}
+          defaultMessage={editMessage ? editMessage.content : draft}


ComposeBox [nfc]: Add defaultTopic and defaultMessage props.

I'm having some trouble verifying this commit as a pure refactor, and I think a main reason is that I'm not sure why editMessage is making an appearance in it, and I don't find it explained.

It seems like there's at least the following change in behavior: if editMessage is populated at the time ComposeBox's constructor is called (before the component's first mount), then the component's state.topic and state.message will be initialized with the data from editMessage, instead of being initialized to something else and then getting changed later via UNSAFE_componentWillReceiveProps.

In practice, at this point in the series, I think that doesn't mean much, because I don't think editMessage will be populated at the time of a first mount of <ComposeBox />. (Is that right?)

But it will be important later, when we put in the key, because first mounts will happen whenever editMessage changes. That would be good to foreshadow in the commit message, if that's the intention—or, I think better, lump the added editMessage conditional in with the key commit because that's where it first becomes useful/necessary (unless there's another use case I'm not thinking of?).

I'm also wondering why, in the case where editMessage is null, is there the following asymmetry between defaultTopic and defaultMessage: The value passed as defaultTopic is an emptiness value (undefined), leaving ComposeBox to fill in a meaningful value, while the value passed as defaultMessage is meaningful (draft).

Is there a reason we'd like to pass something empty for exactly one of { defaultTopic, defaultMessage }, for ComposeBox to fill in? How about both or neither?

I'm having some trouble verifying this commit as a pure refactor, and I think a main reason is that I'm not sure why editMessage is making an appearance in it, and I don't find it explained.

Hmmm, this might be an artifact of the way that I split up these commits. (Or maybe it's needed, I'm not actually sure).

It's basically because of the code in UNSAFE_componentWillReceiveProps, which is editMessage-dependent. Perhaps that doesn't matter for this commit, and should only be added once I remove UNSAFE_componentWillReceiveProps, though?

It's been sort of frustrating for me to try to get this PR into a state with a bunch of atomic commits, since the fundamental thing that I'm trying to fix is that ComposeBox is sort of a mess where changing one thing has a bunch of cascading effects most of the time.

I can also appreciate that it's much easier to review things that are small atomic commits, so I'm happy to work towards that, I'm just not confident in my ability to figure out exactly what each commit does.

I do think you can probably convince yourself that this commit is NFC, though — if editMessage is non-null, that means that UNSAFE_componentWillReceiveProps will run, and so none of what we set in the lines that are changed will matter. I guess that means that I should do that as part of a separate commit, I'll try to fix that up...

I'm also wondering why, in the case where editMessage is null, is there the following asymmetry between defaultTopic and defaultMessage: The value passed as defaultTopic is an emptiness value (undefined), leaving ComposeBox to fill in a meaningful value, while the value passed as defaultMessage is meaningful (draft).

This isn't quite correct, since draft can be undefined — it's possible that both will be unset, or it's possible that just message will be set.

The reason for doing it this way is that we don't save drafts of topic names (except sort of incidentally, in the fact that we store them by narrow). I'm not really sure what the best path forward here is — I think in involves rethinking how drafts work in general.

chrisbobbe · 2021-06-09T14:59:55Z

src/chat/ChatScreen.js

@@ -114,6 +117,26 @@ export default function ChatScreen(props: Props) {
  const sayNoMessages = haveNoMessages && !isFetching;
  const showComposeBox = canSendToNarrow(narrow) && !showMessagePlaceholders;

+  const sendMessage = (


nit: Should use useCallback, I think: #4699 (comment)

Can we write down in our styleguide when we want to use useCallback? I tend to see conflicting reports online, and I don't have a good understanding of what the tradeoffs are.

src/compose/ComposeBox.js

chrisbobbe · 2021-06-09T15:46:39Z

src/chat/ChatScreen.js

@@ -188,6 +188,7 @@ export default function ChatScreen(props: Props) {
          onSend={sendCallback}
          defaultTopic={editMessage ? editMessage.topic : undefined}
          defaultMessage={editMessage ? editMessage.content : draft}
+          autoFocusMessage={editMessage !== null}


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.

chrisbobbe · 2021-06-09T15:53:02Z

src/chat/ChatScreen.js

@@ -188,6 +188,7 @@ export default function ChatScreen(props: Props) {
          onSend={sendCallback}
          defaultTopic={editMessage ? editMessage.topic : undefined}
          defaultMessage={editMessage ? editMessage.content : draft}
+          autoFocusMessage={editMessage !== null}


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

WesleyAC · 2021-06-11T01:16:31Z

I've done some of these changes, will do the rest when I'm back at my computer. Specifically, I think I've done everything except:

Turn Pull sending logic out of ComposeBox into a NFC commit
Use useCallback in src/chat/ChatScreen.js

Let me know if that's incorrect, GitHub's UI is quite unwieldy for me for finding things in a review with this many comments :)

WesleyAC · 2021-06-11T01:19:12Z

(Also, I've not tested all of these changes on-device yet, since the yarn update that I rebased onto seems to have broken the Android build for me, will also do that when I get back)

WesleyAC · 2021-06-11T18:55:22Z

Ok, I believe I've addressed all of your comments @chrisbobbe :)

WesleyAC · 2021-06-17T00:55:10Z

Just rebased this and fixed up a merge conflict. Would love to get this in soon :)

gnprice

Thanks @WesleyAC, and thanks @chrisbobbe for the previous rounds of review! Comments below.

gnprice · 2021-06-23T21:47:44Z

src/chat/ChatScreen.js

+          api
+            .updateMessage(getAuth(getState()), { content, subject }, editMessage.id)


This is a dangerous way to write this code. A UI callback should always use an auth that was gotten at render time, or if that's not possible then an auth that is gotten at the very top of the callback body.

The reason is that when we get the auth from a getState() in the middle of a callback like this, it risks ending up happening after some time has passed, and the auth we get is for a different account from the one the user asked us to take the action on. That isn't currently possible in this version of the code, but it's an easy thing to happen in the future when an await is added somewhere in the logic above this.

The auth is a particularly salient example of this, but the same principle applies to other kinds of data a UI callback might act on: it's important that it be acting on the same data that was reflected in the widget the user interacted with, and the practical way to ensure that is to use data we got at (or before) render time. So in particular this means that where we can avoid it we don't want to be calling getState in a UI callback.

For a class component, the usual way we do this is with a prop provided by connect, like in the existing version of this code.

For a function component, the corresponding way to do it is with useSelector. We can do that here.

src/chat/ChatScreen.js

src/compose/ComposeBox.js

gnprice · 2021-06-23T22:09:08Z

src/compose/ComposeBox.js

 import TextInputReset from 'react-native-text-input-reset';
 import { type EdgeInsets } from 'react-native-safe-area-context';
 import { compose } from 'redux';

+import store from '../boot/store';


BTW this line is a code smell -- it points at basically the issue in my comment at the top.

There might be circumstances where we end up wanting to import the store directly from where it lives as a module global, and to call getState() on it directly. We've talked about potentially doing that in places like the action sheets.

But we currently have zero examples of that -- if you comment out export default store; from src/boot/store.js, the only Flow errors are at the import in StoreProvider.js, and this new one. And for any code that's in or around a React component, there's basically always going to be a better way.

src/compose/ComposeBox.js

gnprice · 2021-06-23T23:11:06Z

src/compose/ComposeBox.js

+  defaultMessage?: string,
+  defaultTopic?: string,


Is "default" an accurate name for these? It feels like "initial" would be more appropriate -- like initialMessage and initialTopic.

In particular, there isn't a particular expectation that this message value is one that the user will actually go with -- it's a draft and probably incomplete, or the existing content of a message they've decided to edit.

Saying something like "initial" would also highlight that their behavior is somewhat unusual for props, in that they get consulted only when the component is first initialized:

state = { // … topic: this.props.defaultTopic !== undefined ? this.props.defaultTopic : /* … */, message: this.props.defaultMessage || '',

Which is good because that's kind of a gotcha in the interface -- it's the reason that it's important that we pass a key that changes when these initial values change. (Or, at the point in the branch where these are introduced: it's not yet a gotcha in the component's interface, but making that the case requires a hack in its implementation, namely the use of UNSAFE_componentWillReceiveProps.)

And in fact we don't have the key change in all the cases where these initial values do -- we have it depend on editMessage but not draft. That gets the right behavior, because we're continuously updating the draft in Redux as the user edits, and the input is uncontrolled so there's no need to change anything to reflect it… but it is odd, so it's good for it to stick out more.

gnprice · 2021-06-23T23:29:29Z

src/compose/ComposeBox.js

    const { message } = this.state;
+    const narrow = this.getDestinationNarrow();


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

I fixed the getDestinationNarrow bug in a new commit, but I think that may actually break editing messages. Looking into this 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.

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

gnprice · 2021-06-23T23:40:19Z

src/compose/ComposeBox.js

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


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.

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.

src/compose/ComposeBox.js

WesleyAC · 2021-06-24T02:37:46Z

Thanks for the review @gnprice! I've addressed everything except the onLayout comment — that's the last commit here, so I'd appreciate a re-review and merging the beginning of the series while I look into that.

chrisbobbe · 2021-06-24T02:53:11Z

src/compose/ComposeBox.js

@@ -336,23 +336,6 @@ class ComposeBox extends PureComponent<Props, State> {
    dispatch(sendTypingStop(narrow));
  };

-  UNSAFE_componentWillReceiveProps(nextProps: Props) {


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.

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

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

Thanks for the explanation! Reworded the commit.

chrisbobbe

Thanks, @WesleyAC, this is getting closer! I think at some point we left this as @gnprice's to review, but I've looked again myself and found some points that I hope will be productive.

chrisbobbe · 2021-07-06T22:44:32Z

src/compose/ComposeBox.js

@@ -363,7 +364,7 @@ class ComposeBox extends PureComponent<Props, State> {

  getDestinationNarrow = (): Narrow => {
    const { narrow } = this.props;
-    if (isStreamNarrow(narrow)) {
+    if (isStreamNarrow(narrow) || isTopicNarrow(narrow)) {


ComposeBox: Fix getDestinationNarrow to use edited topic.

In the commit message, let's write down more explanation of the problem to be fixed, and why we think this is the right way to fix it. If there was a prior discussion, let's link to it.

chrisbobbe · 2021-07-06T22:48:53Z

src/compose/ComposeBox.js

@@ -83,6 +81,9 @@ type Props = $ReadOnly<{|
  editMessage: EditMessage | null,
  completeEditMessage: () => void,

+  initialMessage?: string,


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.

chrisbobbe · 2021-07-06T22:55:22Z

src/compose/ComposeBox.js

@@ -84,6 +84,9 @@ type Props = $ReadOnly<{|
  initialMessage?: string,
  initialTopic?: string,

+  autoFocusTopic: boolean,


(see above about adding jsdoc)

chrisbobbe · 2021-07-06T23:03:47Z

src/chat/ChatScreen.js

-      }
+      if (editMessage) {
+        const content = editMessage.content !== message ? message : undefined;
+        const subject = caseNarrowDefault(


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? 🙂

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.

chrisbobbe · 2021-07-06T23:26:56Z

src/chat/ChatScreen.js

@@ -188,6 +188,7 @@ export default function ChatScreen(props: Props) {
          onSend={sendCallback}
          defaultTopic={editMessage ? editMessage.topic : undefined}
          defaultMessage={editMessage ? editMessage.content : draft}
+          autoFocusMessage={editMessage !== null}


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.

WesleyAC · 2021-07-07T22:22:55Z

@chrisbobbe I addressed all of those comments, should be ready for another look!

gnprice · 2021-07-08T00:21:19Z

src/compose/ComposeBox.js

+    if (isStreamNarrow(narrow) || isTopicNarrow(narrow)) {
      const streamName = streamNameOfNarrow(narrow);
      const topic = this.state.topic.trim();
      return topicNarrow(streamName, topic || '(no topic)');


Does this do the right thing in the case where you're in a topic narrow and simply composing a new message, rather than editing? We don't have a topic input then, so it's not obvious that we would have a state.topic and have the right value there.

It looks like the answer is that it doesn't, as of the commit where this change is made. But then a later change adds more logic to the state.topic initializer so that it does.

Probably the easiest fix is to pull that logic out to its own small prep commit: a commit that makes state.topic always have the topic we're going to send to (if we'd send a stream message at all), which will simplify several changes we want to make in this branch.

It looks like the answer is that it doesn't, as of the commit where this change is made. But then a later change adds more logic to the state.topic initializer so that it does.

I don't think this is true? In this commit, state.topic is set like so:

topic: this.props.lastMessageTopic,

Which we get here:

lastMessageTopic: getLastMessageTopic(state, props.narrow),

So I think state.topic should come from whatever the last message is in props.narrow — in a topic narrow, that should be the correct topic.

Am I missing something here?

Hmm, I guess that gets it closer! I hadn't made that connection.

I think there's still buggy behavior at least latently, in that if you're looking at a topic narrow and it's empty, getLastMessageTopic will return '' instead of the topic from the narrow. I say "latently" because I'm not sure there's a way to navigate to a narrow with no messages in it...

Ah, no, actually I think that '' case will happen all the time. The thing is that because this is the state initializer (in the constructor), it's getting run only when the ComposeBox instance is first created, and doesn't reflect updates to the props. And any time you visit a given narrow for the first time, we'll have nothing in state.narrows at that point.

Anyway. If some chain of reasoning like that does end up finding that the behavior works out correctly, it's definitely the sort of thing I'd want spelled out explicitly 🙂 (And then if it weren't already going away later in the branch, would want to try to find some refactor so that if possible we didn't rely on that kind of non-local connection in steady state.)

Ok, I changed this to use editMessage/isEditing in order to make it more clearly correct

src/compose/ComposeBox.js

gnprice

Thanks for the revisions -- this is looking good! I think my most significant comment is the one above at #4773 (comment) . A couple of nits below.

gnprice · 2021-07-08T04:04:17Z

src/compose/ComposeBox.js

+      this.props.initialTopic !== undefined
+        ? this.props.initialTopic


nit: can use ?? to make this slightly simpler to read

gnprice · 2021-07-08T04:05:30Z

src/compose/ComposeBox.js

+        : isTopicNarrow(this.props.narrow)
+        ? topicOfNarrow(this.props.narrow)
+        : '',
+    message: this.props.initialMessage || '',


nit: ?? seems a bit more precisely what we mean here, though the behavior is the same; also makes it parallel the topic case once that uses ??

This way, if you're editing a message, and you edit the topic, the destination narrow will be the new topic of the message, rather than the topic you happen to be looking at. See discussion on CZO: https://chat.zulip.org/#narrow/stream/48-mobile/topic/Failed.20to.20send.20on.20Android/near/1217320

gnprice

Thanks! Glad to see this moving again 😄

The fix for the getDestinationNarrow issue in the thread above looks good. One nit below, and one other issue -- this commit:
d7960bb ComposeBox: Get edited topic via getDestinationNarrow.

looks like it's an unintended squash of these two previous commits:
e4f71b8 ComposeBox: Get edited topic via getDestinationNarrow.
f0c9022 ComposeBox: Switch message editing to use onSend callback.

gnprice · 2021-08-26T19:49:49Z

src/compose/ComposeBox.js

+    topic:
+      this.props.initialTopic
+      ?? (isTopicNarrow(this.props.narrow) ? topicOfNarrow(this.props.narrow) : ''),
+    message: this.props.initialMessage ?? '',


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.

This will let us move the responsibility to set the initial topic/message to the caller, where it's easier to ensure that the code is correct.

This sends the "stop typing" notification to the destination narrow, rather than the narrow that the ComposeBox happened to be loaded into. This doesn't matter right now, but could in the future when we start sending typing status notifications to streams. 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 is not quite NFC, since getDestinationNarrow will call .trim() on the topic name, whereas just using the topic directly doesn't. However, that's probably for the best, as we generally want to strip the topic name of whitespace (same as the webapp).

This may have a regression that `blur` is not called on the message input box when a message is done being edited, but as far as I can tell, a bug was preventing that blur call from doing anything in the first place. This will also fix a bug that we send a "typing started" notification but never a "typing stopped" notification when editing a message.

This is typically confusing to users, so just remove it and force people to either specify the topic or send to "(no topic)". We should follow up by having the "Topic" input be the default thing that's focused, rather than the "Message" input. Fixes: zulip#4764

The only caller of this was removed in a previous commit, and we don't expect to use it again.

This replaces UNSAFE_componentWillReceiveProps with a key attribute, so that we create a new ComposeBox instance when we or start or stop editing a message. This fixes the "Warning: Cannot update a component from inside the function body of a different component" error that we were getting several hundred times when a user starts editing a message, although we don't fully understand why it fixes this error. The `initialMessage`, `initialTopic`, and `autoFocusMessage` props were added in this commit to emulate the imperative behaviour in UNSAFE_componentWillReceiveProps. See the CZO thread for more info and discussion: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Next.20steps.20for.20.60ComposeBox.60/near/1160847

Since ComposeBox no longer cares which message is being edited, only that a message is being edited, we can simply pass a boolean, rather than an entire EditMessage.

WesleyAC · 2021-08-26T20:05:04Z

Revision pushed, should fix both of those things :)

gnprice · 2021-08-26T20:40:18Z

Thanks, looks good -- merged! 🎉

WesleyAC requested review from gnprice and chrisbobbe June 2, 2021 17:04

This was referenced Jun 2, 2021

ComposeBox: Don't require caughtUp to send in stream narrow. #4772

Closed

ComposeBox: Don't set topic by default in stream narrows. #4769

Closed

WesleyAC force-pushed the composebox-cleanup branch 2 times, most recently from febf8a6 to e06142f Compare June 2, 2021 19:57

chrisbobbe requested changes Jun 4, 2021

View reviewed changes

WesleyAC force-pushed the composebox-cleanup branch from e06142f to acc2ef4 Compare June 4, 2021 22:39

chrisbobbe requested changes Jun 9, 2021

View reviewed changes

WesleyAC force-pushed the composebox-cleanup branch from acc2ef4 to b60892b Compare June 11, 2021 01:02

WesleyAC force-pushed the composebox-cleanup branch from b60892b to 6f77725 Compare June 11, 2021 01:16

WesleyAC force-pushed the composebox-cleanup branch 3 times, most recently from 6f5d9bd to d06cce7 Compare June 11, 2021 18:54

chrisbobbe mentioned this pull request Jun 14, 2021

actionsheet: Allow editing in all narrows #4799

Closed

WesleyAC force-pushed the composebox-cleanup branch from d06cce7 to f9bb55c Compare June 17, 2021 00:54

gnprice reviewed Jun 23, 2021

View reviewed changes

WesleyAC force-pushed the composebox-cleanup branch 2 times, most recently from 965c590 to 7e41e70 Compare June 24, 2021 02:36

chrisbobbe reviewed Jun 24, 2021

View reviewed changes

WesleyAC force-pushed the composebox-cleanup branch from 7e41e70 to a1f6bff Compare June 24, 2021 03:37

WesleyAC force-pushed the composebox-cleanup branch from a1f6bff to 46b989f Compare July 4, 2021 23:36

chrisbobbe reviewed Jul 6, 2021

View reviewed changes

WesleyAC force-pushed the composebox-cleanup branch 3 times, most recently from b08eb04 to 537c8c5 Compare July 7, 2021 22:22

WesleyAC force-pushed the composebox-cleanup branch from 537c8c5 to 4d253d4 Compare July 7, 2021 22:23

gnprice mentioned this pull request Jul 8, 2021

Use more of the screen for @-mention typeahead suggestions #4871

Open

gnprice reviewed Jul 8, 2021

View reviewed changes

WesleyAC force-pushed the composebox-cleanup branch from 4d253d4 to 77a6c96 Compare August 26, 2021 16:57

WesleyAC force-pushed the composebox-cleanup branch 2 times, most recently from de13c24 to e7c5092 Compare August 26, 2021 17:38

gnprice reviewed Aug 26, 2021

View reviewed changes

WesleyAC added 11 commits August 26, 2021 13:02

ComposeBox [nfc]: Add initialTopic and initialMessage props.

1c8aa4b

This will let us move the responsibility to set the initial topic/message to the caller, where it's easier to ensure that the code is correct.

ComposeBox [nfc]: Add autoFocus props.

f262ac8

ComposeBox [nfc]: Pull sending logic out of ComposeBox.

ab9eb3f

topicSelectors [nfc]: Remove getLastMessageTopic.

ec49476

The only caller of this was removed in a previous commit, and we don't expect to use it again.

ComposeBox [nfc]: Stop taking EditMessage prop.

24ada87

Since ComposeBox no longer cares which message is being edited, only that a message is being edited, we can simply pass a boolean, rather than an entire EditMessage.

ComposeBox: Don't save drafts of edited messages.

f8f32a3

WesleyAC force-pushed the composebox-cleanup branch from e7c5092 to f8f32a3 Compare August 26, 2021 20:04

gnprice merged commit f8f32a3 into zulip:master Aug 26, 2021

gnprice mentioned this pull request Sep 13, 2021

Add stream_id to Outbox values #5000

Merged

		api
		.updateMessage(getAuth(getState()), { content, subject }, editMessage.id)

		const { message } = this.state;
		const narrow = this.getDestinationNarrow();

		this.props.initialTopic !== undefined
		? this.props.initialTopic

Composebox cleanup #4773

Composebox cleanup #4773

Conversation

WesleyAC commented Jun 2, 2021 • edited Loading

WesleyAC commented Jun 2, 2021

chrisbobbe left a comment • edited Loading

Choose a reason for hiding this comment

chrisbobbe Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesleyAC Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

WesleyAC commented Jun 4, 2021

WesleyAC commented Jun 4, 2021

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesleyAC commented Jun 11, 2021 • edited Loading

WesleyAC commented Jun 11, 2021

WesleyAC commented Jun 11, 2021 • edited Loading

WesleyAC commented Jun 17, 2021

gnprice left a comment

Choose a reason for hiding this comment

gnprice Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesleyAC Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

WesleyAC commented Jun 24, 2021

chrisbobbe Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Jun 24, 2021 • edited Loading

WesleyAC commented Jun 2, 2021 •

edited

Loading

chrisbobbe left a comment •

edited

Loading

chrisbobbe Jun 3, 2021 •

edited

Loading

chrisbobbe Jun 9, 2021 •

edited

Loading

chrisbobbe Jul 7, 2021 •

edited

Loading

chrisbobbe Jun 4, 2021 •

edited

Loading

chrisbobbe Jun 4, 2021 •

edited

Loading

WesleyAC Jun 4, 2021 •

edited

Loading

chrisbobbe Jun 4, 2021 •

edited

Loading

chrisbobbe Jun 9, 2021 •

edited

Loading

chrisbobbe Jun 9, 2021 •

edited

Loading

chrisbobbe Jun 9, 2021 •

edited

Loading

WesleyAC commented Jun 11, 2021 •

edited

Loading

WesleyAC commented Jun 11, 2021 •

edited

Loading

gnprice Jun 23, 2021 •

edited

Loading

WesleyAC Jun 24, 2021 •

edited

Loading

chrisbobbe Jun 24, 2021 •

edited

Loading

chrisbobbe Jun 24, 2021 •

edited

Loading

chrisbobbe Jun 24, 2021 •

edited

Loading

WesleyAC Jul 8, 2021 •

edited

Loading