Internal links ignore /near/MSGID pointing to specific messages

[gnprice]

@rishig reports:

The following link is sending me to the end of the narrow on mobile, i.e. it seems to be ignoring the /near/779699 part:
https://chat.zulip.org/#narrow/stream/14-GSoC/topic/Zulip.20swag/near/779699

This is a kind of followup to #2760 -- just a few weeks ago we made such links work at all, going to the right narrow, but the near part which should point to a specific message isn't working.

Looking in the code, a link in the message list is handled here:

export const messageLinkPress = (href: string) => (dispatch: Dispatch, getState: GetState) => {
  // ...
  const narrow = getNarrowFromLink(href, auth.realm, usersById, streamsById);
  if (narrow) {
    const anchor = getMessageIdFromLink(href, auth.realm);
    dispatch(doNarrow(narrow, anchor));
    return;

But then the only thing that doNarrow does with its anchor parameter is to make sure to fetch messages around that desired anchor:

export const doNarrow = (narrow: Narrow, anchor: number = FIRST_UNREAD_ANCHOR) => (
  // ...
  dispatch({ type: DO_NARROW, narrow });
  dispatch(fetchMessagesInNarrow(narrow, anchor));
  dispatch(navigateToChat(narrow));
};

It doesn't actually do anything to try to place our scroll position there.

Looking through the history, it seems like in fact this code may never have worked. Certainly as far back as commit fcff285 a bit over two years ago, when messageLinkPress itself was created in a small refactoring, the anchor parameter has been computed and then mostly ignored; in fact at that point, it was completely ignored, not even used for trying to fetch the right messages.

[gnprice]

(Moving this from the issue description, to add a bit of structure.)

For how to go about implementing proper behavior for these links, here's some background from reading the code:

• We already have some code in and around the webview that's intended to do something like this. It just isn't being invoked in the case of following a link -- some of it isn't currently ever invoked, and I'm not sure if any of it is. So that code may be helpful, though it should be read with a bit of skepticism because it hasn't been getting exercised in a long time.
• Specifically, in js.js there is scrollToAnchor, which gets invoked by handleUpdateEventContent if uevent.updateStrategy === 'scroll-to-anchor'.
• Looking for where that updateStrategy property gets set, it's in webViewHandleUpdates.js in updateContent, from the return value of getMessageUpdateStrategy. Which in turn has this conditional:

   } else if (
    !transitionProps.sameNarrow
    || transitionProps.allNewMessages
    || transitionProps.messagesReplaced
  ) {
    return 'scroll-to-anchor';

• More investigation needed to understand just when that condition will be true.
• Meanwhile the anchor property on the same WebViewUpdateEventContent object, which is what controls the actual anchor to scroll to, is set like so:

    anchor: nextProps.anchor,

• Which comes from here, in the connect call around MessageList:

    anchor: props.anchor !== undefined ? props.anchor : getAnchorForNarrow(props.narrow)(state),

• And props.anchor, in turn, is passed by SearchMessagesCard but never by the Chat component, which is the caller of MessageList for all message lists other than search.

So a reasonable strategy would be:

• Get the scrollToAnchor code to run; check that it seems to work, and fix (or replace) it if not. At this stage you might do something ad hoc to invoke it, like hardcode something into updateContent. The goal is to ensure that js.js does the right thing, provided it's asked to.
• Once that's working, try to get all the layers up through MessageList.js to do the right thing, provided they're asked to. You might again do something ad hoc to invoke this, like hardcode an anchor prop into the MessageList call in Chat.js.
  • This will involve some reading through and sorting out of what getMessageUpdateStrategy and getMessageTransitionProps and so on are doing.
• Then we just need to get Chat.js to pass the right props to MessageList. I think this will be relatively easy: make Chat take another navigation parameter like anchor: number | null, make navigateToChat take the same parameter and pass it through, and make doNarrow pass it that parameter.
• There may be some fiddling required for the fact that it looks like both doNarrow and that connect call around MessageList have their own ideas of what anchor should default to -- respectively FIRST_UNREAD_ANCHOR and getAnchorForNarrow(props.narrow)(state).

[chrisbobbe]

One good place to start would be the TODO in src/message/messagesActions.js:

// TODO: Use anchor to open the message list to a particular message.

[chrisbobbe]

See CZO where I described a latent bug where the message list shows messages consecutively, when they aren't actually consecutive.

And some brainstorming for a solution: we need to better keep the invariant that a state.narrows[narrowKey] is a contiguous block of messages. (If the message IDs in the narrowKey narrow are [1, 2, 3, 4, 5], then state.narrows[narrowKey] can be [1, 2, 3, 4, 5], or [1, 2, 3], or [3, 4, 5], etc., but it can't be [1, 2, 4, 5] or [1, 5], etc.)

[gnprice]

Reported again today in chat by @jyn514:

[…] the said link opens the same topic again, but doesn't scroll to the right message, so it effectively does nothing

It also adds the topic to the history, so the "back" action stays on the same topic instead of behaving the same as the left arrow in the top left corner

Probably the main symptom of this issue is when people follow the "said" links inserted by quote-and-reply. They work OK if the linked-to conversation is very short; but if it's longer, and especially if the link is within the same conversation -- which is very common for quote-and-reply "said" links -- then they're not very helpful until we resolve this issue.

I also just looked at #4377, from early this year, and I believe it's another report of that same symptom.