Mostly handle topic edits / stream moves #5259

gnprice · 2022-02-24T00:02:16Z

The main things this does are:

Fix an existing bug where if you have a ChatScreen and all the messages go away, we'll show NoMessages and get stuck there (until the user navigates out of the screen and back in), even if our state.caughtUp correctly says that we actually don't know if there are any messages in the narrow.
- This is a bit of an edge case today, but would get much more easily triggered after the other changes.
Update state.narrows on topic/stream edits #2688
Update narrow on topic move #5251, or rather the most conspicuous part of it; I'll open followup issues for the rest.

Fixes: #2688
Fixes: #5251

chrisbobbe

Thanks, glad to have this! See a few comments below.

chrisbobbe · 2022-02-24T01:34:44Z

src/__tests__/lib/exampleData.js

 } from '../../api/modelTypes';
 import { makeUserId } from '../../api/idTypes';
 import type { InitialData } from '../../api/apiTypes';
+import { EventTypes, type UpdateMessageEvent } from '../../api/eventTypes';


example data: Add mkActionEventUpdateMessage

nit: EventTypes is an unused import at this commit

chrisbobbe · 2022-02-24T01:58:48Z

src/message/messagesReducer.js

-          : messageWithNewCommonFields;
-      });
+      // TODO(#3408): Handle these.
+      return state;


message: Stop maintaining edit history we never show

Ah, too bad. I guess we should have a TODO(#4134): in here too? #4134

At one point I think #4173 might have been close, but I could be mis-remembering, and it's gathered conflicts (and doesn't specially handle topic/stream moves at all).

(Ah never mind; I see we mention #4134 in a later commit.)

However we shouldn't sweep away the following user-visible functionality in this commit 😛:

Message content (.content) reacts to message-content edits

Last-edited time (.last_edit_timestamp) reacts to all edits

Message content (.content) reacts to message-content edits

Hmm yikes, good catch!

And yeah, I guess we also show whether and when the message was edited. Will fix.

chrisbobbe · 2022-02-24T02:24:59Z

src/message/messagesReducer.js

+          // TODO(i18n): Not sure this "unknown" ever reaches the UI, but
+          //   it'd be nice to translate it in case it can.


I'm not sure if this is what you're suggesting, but is there a precedent for keeping a translated UI string in Redux? I'm not sure how feasible this is in general. To keep the reducers pure, we'd need each of the inputs for our translation functions (intl.formatMessage, intl.formatDate, etc.) to be either constant, or present in the state or relevant action(s). I think we'd start by making this reducer aware of SET_GLOBAL_SETTINGS actions that want to change state.settings.language.

We still couldn't take full advantage of react-intl. For example, we couldn't have a specially formatted React element appear in the output as we do in LegalScreen, because those objects don't belong in Redux (or haven't so far):

label={{ text: 'Terms for {realmName}', values: { realmName: <ZulipText style={{ fontWeight: 'bold' }} text={realmName} /> }, }}

I could also imagine a world where we don't want to keep the language selection in Redux at all. Maybe one day the language selection/data will flow from some iOS/Android API, if it turns out that platforms prefer that? (Though I guess as far as we use that data in JS-land, we'll have gotten it over the RN bridge, which means it's serializable and we can store a copy in Redux if we want.)

It'd be awkward in this case if there's a real, known stream with the name "unknown". In principle we wouldn't want that to be translated.

Under the "normalized data" strategy being considered in #5208 (comment), we have this:

On receiving a message (either from a get-messages request, or a message event), if it's in a stream we don't already know about, go update the streams data with what the message tells us about the stream (i.e., its name.)

If we're going with that strategy (in which display_recipient on a stream message never gets shown in the UI), I wonder if the thing to do here is update the streams data with…the fact that we know about this other stream, but we only know its ID, not anything else, not even its name…😝

I'm not sure if this is what you're suggesting, but is there a precedent for keeping a translated UI string in Redux? I'm not sure how feasible this is in general. To keep the reducers pure, we'd need

Yeah, this sort of reason is why I didn't attempt to actually do this 😉

If we had some reasonably simple way to get a translation with the current choice of language, I think because this is already an edge case it could be an acceptable hack here to use that and have the name not get updated if the language choice changes.

Under the "normalized data" strategy

Yeah, so one consequence of that strategy would be that this lookup would happen in UI code when we're about to present the stream name to the user, instead of here in this reducer. That would make it straightforward to translate the string we use for "unknown", and we naturally would.

chrisbobbe · 2022-02-24T03:31:11Z

src/chat/ChatScreen.js

+  //   isFetching false, even though the fetch effect will cause a rerender
+  //   with isFetching true.  It'd be nice to avoid that.
+  const nothingKnown = messages.length === 0 && !caughtUp.older && !caughtUp.newer;
+  useEdgeTriggeredEffect(scheduleFetch, nothingKnown, true);


I'm interested in why to pass true for includeStart in this useEdgeTriggeredEffect call.

It means we'll schedule a fetch on first mount…but in the useEffect just below this (you've commented with "On first mount, fetch"), we'll do a fetch and unschedule the one we just scheduled here.

I think that means that we'll do the same fetches whether we pass true or false for includeStart, and I'm curious if there's a right answer for which we should pass.

Looking again, do we actually quite want the existing useEffect that fetches unconditionally on first mount? What if, on first mount, we already have a screenful or so of messages to show? Or if we know we're caught up with the whole narrow already?

Yeah, so ideally I think these two (on first mount, and edge-triggered on nothingKnown) would be consolidated. In this PR it comes out this way because I (a) didn't want to disturb what happens on first mount (because that currently works at least OK, and this is a tricky area to get right), and (b) wanted something relatively simple and narrowly-targeted for this additional case, to avoid inadvertently causing disruption in cases where things had been working smoothly.

Point (a) meant the first-mount fetch would stay broad, and point (b) meant the new edge-triggered fetch would be narrow, so that blocks combining them.

The existing thing isn't actually an unconditional fetch, even though it looks that way from this code. Over in fetchActions.js, it ends up conditioned on this before actually doing any fetching:

// Ideally this would detect whether, even if we don't have *all* the // messages in the narrow, we have enough of them around the anchor // to show a message list already. For now it's simple and cautious. const caughtUp = getCaughtUpForNarrow(state, narrow); return !(caughtUp.newer && caughtUp.older);

So if we know we've fetched the whole narrow already, we don't re-fetch. Otherwise we fetch. And the comment speaks to your question about if we already have a screenful or so of messages.

One reason I wanted (b), to keep the new thing narrowly targeted, is that when we do one of these fetches it replaces whatever else we knew in that narrow: see the logic around replaceExisting in narrowsReducer, and note that these fetches have anchor FIRST_UNREAD_ANCHOR. So if the new thing were to have a condition that sometimes triggered when the messages around the first unread didn't include the messages you'd been looking at, then that could be disruptive.

The reason I pass true for includeStart here is just that within this condition's own logic, if we know nothing about the narrow's messages then that's just as good a reason to fetch following first mount as it is later. It is redundant with the first-mount fetch below, but harmlessly so -- just like the eventQueueId line -- so it's free to follow that internal logic and not specially avoid fetching in that case.

chrisbobbe · 2022-02-24T20:07:47Z

src/caughtup/__tests__/caughtUpReducer-test.js

+          // old topic narrow remains caught up
+          [mkKey(eg.stream, topic1), { older: true, newer: true }],
+          // new topic narrow gets cleared
+          [mkKey(eg.stream), { older: true, newer: true }], // stream narrow unchanged


Yeah, kind of tricky to format these comments for clarity, right? Some of them are annotating items in the array (the first and last comments), and some are explaining why we've omitted an item in the array (the middle comment). I wonder if we can be clearer at a glance which purpose is meant for each comment?

These are some alternatives that you've probably considered, that have tradeoffs for horizontal or vertical space:

objectFromEntries([ [mkKey(eg.stream, topic1), { older: true, newer: true }], // old topic narrow remains caught up // new topic narrow gets cleared [mkKey(eg.stream), { older: true, newer: true }], // stream narrow unchanged ])

objectFromEntries([ // old topic narrow remains caught up [mkKey(eg.stream, topic1), { older: true, newer: true }], // new topic narrow gets cleared // stream narrow unchanged [mkKey(eg.stream), { older: true, newer: true }], ])

It may be that the current formatting ends up being an OK balance but I thought I'd point it out.

Hmm, yeah, on rereading I see this formatting is kind of confusing -- thanks for flagging it.

I think the core of what makes it confusing may be the mixed use of a comment at the end of the line, and a comment on the preceding line. I've fixed that and made other tweaks; take a look and see if it seems clearer.

chrisbobbe · 2022-02-24T20:12:08Z

src/caughtup/__tests__/caughtUpReducer-test.js

+          // old topic and stream narrows remain caught up
+          [mkKey(eg.stream, topic1), { older: true, newer: true }],
+          [mkKey(eg.stream), { older: true, newer: true }],
+          // new topic narrow and stream narrow both cleared


See my previous comment on formatting these comments; I guess we could consider these alternatives:

objectFromEntries([ // old topic and stream narrows remain caught up [mkKey(eg.stream, topic1), { older: true, newer: true }], [mkKey(eg.stream), { older: true, newer: true }], // new topic narrow and stream narrow both cleared ])

objectFromEntries([ [mkKey(eg.stream, topic1), { older: true, newer: true }], // old topic narrow remains caught up [mkKey(eg.stream), { older: true, newer: true }], // old stream narrow remains caught up // new topic narrow and stream narrow both cleared ])

chrisbobbe · 2022-02-24T20:23:27Z

src/chat/narrowsReducer.js

+        result = addMessages(result, topicNarrow(new_stream_id, new_topic), event.message_ids);
+        result = removeMessages(result, topicNarrow(orig_stream_id, orig_topic), messageIdSet);
+        if (new_stream_id !== orig_stream_id) {
+          result = addMessages(result, streamNarrow(new_stream_id), event.message_ids);


I know that addMessages doesn't currently do anything with its messageIds param anyway (that'll change when we address the TODOs), but is there a reason not to give it messageIdSet instead of the event.message_ids array, for consistency between addMessages and removeMessages?

When adding the messages, we'll want them in sorted order. Partly because that's the order in which we want them in the result, and partly because it lets us quickly look at the first and last one to answer questions like "are these all contained in the interval where we know we have all this narrow's messages".

chrisbobbe · 2022-02-24T20:30:15Z

src/caughtup/caughtUpReducer.js

+  // eslint-disable-next-line no-unused-vars
+  const { [key]: ignored, ...rest } = state;
+  return rest;


I see what this is doing (and, after reading the narrows reducer, I even understand why we're deleting something in a function named "add messages" 😅)—but it's a little awkward, isn't it, compared to the simpler state.delete(key) in the narrows reducer?

Would it be pretty quick and easy to make CaughtUpState an Immutable.Map, maybe as a followup?

It is. And yeah, that'd probably be a good change.

chrisbobbe · 2022-02-24T21:17:05Z

src/events/doEventActionSideEffects.js

+        }
+
+        // $FlowFixMe[incompatible-type]: relying on ChatScreen route-params type
+        const narrow: Narrow = route.params?.narrow;


I think optional chaining isn't necessary here. We know route.name is 'chat', and the 'chat' route always has params (with narrow: Narrow). It'd be a bug if we had a 'chat' route where params was missing.

Yeah, agreed, thanks.

chrisbobbe · 2022-02-24T21:33:26Z

src/events/doEventActionSideEffects.js

+          // messages, in case only some of the conversation was moved.
+          // We don't have a blue box, but:
+          // TODO: Ideally if the moved messages are all offscreen we'd skip this.
+          NavigationService.dispatch({


This may end up being an appropriate use of NavigationService; I think it'd fall roughly under the third bullet point in #4417 (comment):

The callsites for the action creators in navActions seem to fall into three categories:

[…]

[…]

We're not in a React component; e.g., we're somewhere deep in the event-handling code for a button being pressed on the action sheet, or for an outbound event coming from the WebView. It might actually end up being appropriate to keep the use of NavigationService for these callsites (as an "advanced use case"), rather than finding a way to thread the navigation object through from React. But we may find that something forces our hand.

(I guess that issue is specifically about pruning down navActions, but the motivation for that is that we don't like to use NavigationService. I still don't feel like I understand its behavior quirks, but I've encountered one lately: 6b06584, a commit in the draft PR #5197.)

If we encounter bugs, one alternative that might be explored is to use navigation.setParams in the ChatScreen component itself. That'd take some rewiring of how we convey that messages have been moved.

I think it'd fall roughly under the third bullet point in #4417 (comment):

Yeah, I think it does fall basically in that category.

Hmm, one related thing that's going to require some care here in the future is that when we're processing events for accounts that aren't the active one in the UI, we'll want to do this only for the active one. I'll add a TODO-5005 comment to help avoid forgetting that.

gnprice · 2022-02-25T23:47:41Z

Thanks for the review! Just pushed a revision. The most interesting part is naturally in messagesReducer, related to here: #5259 (comment)

This should be NFC as to the action type EventUpdateMessageAction.

At the moment this makes only a tiny simplification to its two call sites. But we're going to make some changes to this action that make it somewhat more complex, and this will simplify those.

We're going to add another property here, to carry some precomputed information that multiple reducers will want. Seems cleanest to do that separately from the event, rather than mixing more properties into it. For the moment this structure looks duplicative of the `type: EVENT` case, but that will change when we add another property.

We'll want to reuse this bit of logic in multiple sub-reducers, so it's time it got factored out from this one. And in fact putting it inside the API code seems most appropriate: it's basically part of presenting an improved version of what the API *could* have been, if designed from the outset with knowledge of all the features that would be added.

This makes it available to all reducers while only calling it once.

Seems like ideally the server would sort these before returning them, and guarantee that in the API. But that doesn't appear to currently be guaranteed by the implementation: in `zerver.lib.actions.do_update_message`, the list comes from `zerver.lib.topic.update_messages_for_topic_edit`, and that does a database query with no ORDER BY, and doesn't itself sort either. So, guarantee it ourselves at the point of entry.

This simplifies things for adding some logic here that *does* have user-visible effect, namely updating the stream and topic. We can always add back later some code to maintain this history. We'll want to do that before we add any UI to show that history.

The two migrations here (of the property's name, and its type when present) are parallel to migrations in the `update_message` event. Ever since the rename (back in Zulip Server 3.0), we've been getting message objects with a `topic_links` and no `subject_links`; and then if the message is ever updated we add `subject_links: undefined` to it. The reason this API change hasn't surfaced as a bug is that we never actually consume this property (under either name). For the moment, just cut out the broken code rather than update it. Properly handling the new situation will be more than a trivial change, because we'll want to canonicalize how we store this property internally.

We'll start using this shortly.

This handles part of zulip#3408, closely related to zulip#2688. In particular this means that if you find one of these messages in an interleaved view where recipient headers are shown, the recipient header will take you to the correct narrow. We actually did handle topic edits, but only for a single message, and didn't handle moves between streams.

In particular this avoids the rather action-at-a-distance relationship between the effect above that schedules a fetch when eventQueueId changes, and the need for eventQueueId to appear in this later effect's dependency list.

We'll use this to schedule a fetch in some other circumstances.

There's a bug that's currently somewhat hard to trigger: if you're looking at some narrow, and then all the messages that we have in that narrow disappear, we'll switch to saying "No messages" -- and won't attempt any fetching of more messages -- even when the truth is that we just don't know if there are any messages. The user ends up having to navigate out and come back, and only then do we go fetch messages. This occurs even though our `state.caughtUp` will correctly say that we don't know if there are any messages. Currently this can happen if all the messages are deleted (perhaps you're looking at a short conversation), or if you're looking at the starred-messages or @-mentions narrow and the messages all lose the respective flag. As we start supporting moving a message / editing its topic and stream, though, this will become more common. In this fix, we make sure that at least we start fetching messages when this happens. The fetching will also replace the "No messages" with placeholders showing we're fetching. There may still be a flash of the untrue "No messages" message, but only for a frame or two while the component rerenders -- it doesn't wait for the fetch request, let alone stay stuck there indefinitely.

This isn't optimal in that if, say, a conversation is moved, we'll have to refetch the messages in the new topic to discover that it includes the messages that were just moved. But it is *correct* -- we won't mistakenly think we know something we actually don't, and in particular won't get stuck in a state of such wrong information. And if the user looks at that conversation, or is already looking, then we will indeed make that fetch. The reason keeping things correct is tricky is that the target stream and topic may already have some messages -- in fact this is a fairly common use case. We may not know anything about that narrow, and so we may not be able to tell if that's the case. Leave some TODO comments about ways we could use knowledge we have when applicable, though. Fixes: zulip#2688

This covers the most important bit: when applicable, the narrow the ChatScreen is looking at gets updated to follow the moved messages. Left open are a couple of other things to do, related to the compose box. I think I'm content to close the issue without those, and open follow-up issues for them. Include TODO comments for them. Fixes: zulip#5251

chrisbobbe · 2022-03-02T01:51:04Z

Thanks, LGTM! Merged.

chrisbobbe requested changes Feb 24, 2022

View reviewed changes

gnprice force-pushed the pr-topic-move branch from 0c722aa to fe2bf47 Compare February 25, 2022 23:46

chrisbobbe force-pushed the pr-topic-move branch from fe2bf47 to 6b67d3b Compare March 2, 2022 01:36

gnprice and others added 16 commits March 1, 2022 17:39

api types: Move update_message event into a proper event type

de76082

This should be NFC as to the action type EventUpdateMessageAction.

example data: Add mkActionEventUpdateMessage

c3f0e70

At the moment this makes only a tiny simplification to its two call sites. But we're going to make some changes to this action that make it somewhat more complex, and this will simplify those.

event [nfc]: Compute messageMoved early on update_message

8d6c1a3

This makes it available to all reducers while only calling it once.

message [nfc]: Take whole per-account state in reducer

4622dd0

We'll start using this shortly.

ChatScreen [nfc]: Factor out small callback scheduleFetch

9d543c3

We'll use this to schedule a fetch in some other circumstances.

reactUtils: Add useEdgeTriggeredEffect

bc81c42

chrisbobbe force-pushed the pr-topic-move branch from 6b67d3b to 1a45db2 Compare March 2, 2022 01:42

chrisbobbe merged commit 1a45db2 into zulip:main Mar 2, 2022

gnprice deleted the pr-topic-move branch March 2, 2022 04:32

chrisbobbe mentioned this pull request Mar 8, 2022

Stop event-polling for an account when it's logged out #5161

Merged

		// TODO(i18n): Not sure this "unknown" ever reaches the UI, but
		// it'd be nice to translate it in case it can.

Uh oh!

Uh oh!

Mostly handle topic edits / stream moves #5259

Mostly handle topic edits / stream moves #5259

Uh oh!

Conversation

gnprice commented Feb 24, 2022

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Feb 25, 2022

Uh oh!

chrisbobbe commented Mar 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chrisbobbe Feb 24, 2022 •

edited

Loading

chrisbobbe Feb 24, 2022 •

edited

Loading