Skip to content

Follow /near/ links through topic moves/renames #5306

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.

Sign up for GitHub

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

Jump to bottom
Closed
gnprice opened this issue Mar 22, 2022 · 2 comments · Fixed by #5496
Closed

Follow /near/ links through topic moves/renames #5306

gnprice opened this issue Mar 22, 2022 · 2 comments · Fixed by #5496
Assignees
@chrisbobbe
Labels
P1 high-priority server release goal Things we should try to coordinate with a major Zulip Server release. webapp parity Features that exist in the webapp that we need to port to mobile. We aren't aiming for full parity. @zulip/shared Good spots to share code with the webapp

Comments

@gnprice
Copy link
Member

gnprice commented Mar 22, 2022

This is a new feature on web in zulip/zulip#21426 / zulip/zulip#15290, fixing a regular pain point with resolved topics in particular. We should adopt it on mobile too.

To implement it, we'll probably want to move some of the implementation into shared code so we can reuse it. In particular message_edit.stream_and_topic_exist_in_edit_history:
zulip/zulip@2a48528

Note that we'll also need to resume maintaining the edit_history property on messages. Currently in messageReducer.js we have, since 055f45d:

          ...(oldMessage: M),
          content: event.rendered_content ?? oldMessage.content,
          last_edit_timestamp: event.edit_timestamp ?? oldMessage.last_edit_timestamp,
          // TODO(#3408): Update edit_history, too.  This is OK for now
          //   because we don't actually have any UI to expose it: #4134.
@gnprice gnprice added P1 high-priority @zulip/shared Good spots to share code with the webapp webapp parity Features that exist in the webapp that we need to port to mobile. We aren't aiming for full parity. server release goal Things we should try to coordinate with a major Zulip Server release. labels Mar 22, 2022
@chrisbobbe
Copy link
Contributor

See details of a recent API change around edit_history: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/edit.20history/near/1342402

@chrisbobbe chrisbobbe mentioned this issue Jun 22, 2022
Merged
@gnprice gnprice mentioned this issue Jul 14, 2022
Merged
@chrisbobbe chrisbobbe mentioned this issue Jul 27, 2022
Closed
@chrisbobbe
Copy link
Contributor

Here's a description of an algorithm to implement, on CZO:

When a user clicks a /near/ link in a message,

  1. Get the message, either from our local data structures or (failing that) from a single-message fetch, using the API added in api: Send full message in GET /messages/{message_id} response. zulip#21391.
  2. If the message is currently in the stream and topic encoded in the /near/ link, open the topic narrow for that stream/topic, and scroll to the message*.
  3. If the message was at some point in the stream and topic encoded in the /near/ link, according to its edit_history, open the topic narrow for the stream/topic that the message is currently in, and scroll to the message*.
  4. Otherwise (i.e., the message was never in the stream/topic encoded in the /near/ link), open the topic narrow for the stream/topic encoded in the link, and scroll to the message "nearest" to the message ID*.

*Actually opening a narrow scrolled to a specific message ID is blocked on #M3604.

with

Yeah, I think it'd be very reasonable to not do special handling for case 4 at first, leaving a comment and an open issue to record that it's a corner case we don't handle perfectly.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 11, 2022
@chrisbobbe
api: Add getSingleMessage binding for GET messages/{message_id}
4fee291 
We'll use this for zulip#5306; see the plan in discussion:
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M5306.20Follow.20.2Fnear.2F.20links.20through.20topic.20moves.2Frenames/near/1407930

In particular, we want the stream and topic for a stream message
that we fetch using this endpoint (which we'd do as a fallback for
when we don't already have the message in our data structures.)
@chrisbobbe chrisbobbe mentioned this issue Aug 11, 2022
Merged
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 11, 2022
@chrisbobbe
api: Add getSingleMessage binding for GET messages/{message_id}
8c17242 
We'll use this for zulip#5306; see the plan in discussion:
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M5306.20Follow.20.2Fnear.2F.20links.20through.20topic.20moves.2Frenames/near/1407930

In particular, we want the stream and topic for a stream message
that might not be in our data structures. We'll use this endpoint to
fetch that information.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 17, 2022
@chrisbobbe
internalLinks [nfc]: Fix bug in getMessageIdFromLink
4d5c073 
NFC because we always drop the result of this function on the floor,
after passing it to doNarrow. But we need this to start giving the
right answer, for zulip#5306, "Follow /near/ links through topic
moves/renames".
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 17, 2022
@chrisbobbe
internalLinks [nfc]: Fix bug in getMessageIdFromLink
d9b5a8f 
There were two things wrong here:

- `url.includes('near')` was a pretty poor heuristic to decide if
  the narrow link has a "near" operator. It would give false
  positives if "near" appeared somewhere other than the
  operator-operand pair "/near/{message_id}", e.g., if it appeared
  in the realm's base URL (if `url` is an absolute URL string), or a
  stream or topic name in an operand.

- Once we (unsoundly) decided that there was a "near" operator, we'd
  try to locate it in hashSegments by searching *all* of
  hashSegments, including ones that represent operands. This could
  be thrown off by an operand with the value "near", as in
  `topic/near`.

NFC because we always drop the result of this function on the floor,
after passing it to doNarrow. But we need this to start giving the
right answer, for zulip#5306, "Follow /near/ links through topic
moves/renames".
@chrisbobbe chrisbobbe mentioned this issue Aug 17, 2022
Merged
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 17, 2022
@chrisbobbe
internalLinks [nfc]: Fix bug in getMessageIdFromLink
7b23de1 
There were two things wrong here:

- `url.includes('near')` was a pretty poor heuristic to decide if
  the narrow link has a "near" operator. It would give false
  positives if "near" appeared somewhere other than the
  operator-operand pair "/near/{message_id}", e.g., if it appeared
  in the realm's base URL (if `url` is an absolute URL string), or a
  stream or topic name in an operand.

- Once we (unsoundly) decided that there was a "near" operator, we'd
  try to locate it in hashSegments by searching *all* of
  hashSegments, including ones that represent operands. This could
  be thrown off by an operand with the value "near", as in
  `topic/near`.

NFC because we always drop the result of this function on the floor,
after passing it to doNarrow. But we need this to start giving the
right answer, for zulip#5306, "Follow /near/ links through topic
moves/renames".
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 29, 2022
@chrisbobbe
internalLinks [nfc]: Fix bug in getMessageIdFromLink
25ab7c4 
There were two things wrong here:

- `url.includes('near')` was a pretty poor heuristic to decide if
  the narrow link has a "near" operator. It would give false
  positives if "near" appeared somewhere other than the
  operator-operand pair "/near/{message_id}", e.g., if it appeared
  in the realm's base URL (if `url` is an absolute URL string), or a
  stream or topic name in an operand.

- Once we (unsoundly) decided that there was a "near" operator, we'd
  try to locate it in hashSegments by searching *all* of
  hashSegments, including ones that represent operands. This could
  be thrown off by an operand with the value "near", as in
  `topic/near`.

NFC because we always drop the result of this function on the floor,
after passing it to doNarrow. But we need this to start giving the
right answer, for zulip#5306, "Follow /near/ links through topic
moves/renames".
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 29, 2022
@chrisbobbe
internalLinks [nfc]: Fix bug in getMessageIdFromLink
3daa56c 
There were two things wrong here:

- `url.includes('near')` was a pretty poor heuristic to decide if
  the narrow link has a "near" operator. It would give false
  positives if "near" appeared somewhere other than the
  operator-operand pair "/near/{message_id}", e.g., if it appeared
  in the realm's base URL (if `url` is an absolute URL string), or a
  stream or topic name in an operand.

- Once we (unsoundly) decided that there was a "near" operator, we'd
  try to locate it in hashSegments by searching *all* of
  hashSegments, including ones that represent operands. This could
  be thrown off by an operand with the value "near", as in
  `topic/near`.

NFC because we always drop the result of this function on the floor,
after passing it to doNarrow. But we need this to start giving the
right answer, for zulip#5306, "Follow /near/ links through topic
moves/renames".
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 6, 2022
@chrisbobbe
api: Add getSingleMessage binding for GET messages/{message_id}
ec8c22d 
We'll use this for zulip#5306; see the plan in discussion:
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M5306.20Follow.20.2Fnear.2F.20links.20through.20topic.20moves.2Frenames/near/1407930

In particular, we want the stream and topic for a stream message
that might not be in our data structures. We'll use this endpoint to
fetch that information.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 7, 2022
@chrisbobbe
api: Add getSingleMessage binding for GET messages/{message_id}
598ec8d 
We'll use this for zulip#5306; see the plan in discussion:
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M5306.20Follow.20.2Fnear.2F.20links.20through.20topic.20moves.2Frenames/near/1407930

In particular, we want the stream and topic for a stream message
that might not be in our data structures. We'll use this endpoint to
fetch that information.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 13, 2022
@chrisbobbe
api: Add getSingleMessage binding for GET messages/{message_id}
c35b883 
We'll use this for zulip#5306; see the plan in discussion:
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M5306.20Follow.20.2Fnear.2F.20links.20through.20topic.20moves.2Frenames/near/1407930

In particular, we want the stream and topic for a stream message
that might not be in our data structures. We'll use this endpoint to
fetch that information.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 23, 2022
@chrisbobbe
messagesActions: Follow tapped /near/ links through message moves
bc1a4a8 
Implementing the algorithm described in the issue and on CZO:
  zulip#5306 (comment)

> When a user clicks a `/near/` link in a message,
>
> 1. Get the message, either from our local data structures or
>    (failing that) from a single-message fetch, using the API added
>    in zulip/zulip#21391.
> 2. If the message is currently in the stream and topic encoded in
>    the `/near/` link, open the topic narrow for that stream/topic,
>    and scroll to the message*.
> 3. If the message was *at some point* in the stream and topic
>    encoded in the `/near/` link, according to its `edit_history`,
>    open the topic narrow for the stream/topic that the message is
>    currently in, and scroll to the message*.
> 4. Otherwise (i.e., the message was never in the stream/topic
>    encoded in the `/near/` link), open the topic narrow for the
>    stream/topic encoded in the link, and scroll to the message
>    "nearest" to the message ID*.
>
> *Actually opening a narrow scrolled to a specific message ID is
> blocked on zulip#3604.

It actually turned out quite easy to do the special handling for
part 4, so that's included here. (The bit about blocking on zulip#3604 is
still true.)

Fixes: zulip#5306
@chrisbobbe chrisbobbe mentioned this issue Sep 23, 2022
Merged
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 23, 2022
@chrisbobbe
messagesActions: Follow tapped /near/ links through message moves
a78a4a0 
Implementing the algorithm described in the issue and on CZO:
  zulip#5306 (comment)

> When a user clicks a `/near/` link in a message,
>
> 1. Get the message, either from our local data structures or
>    (failing that) from a single-message fetch, using the API added
>    in zulip/zulip#21391.
> 2. If the message is currently in the stream and topic encoded in
>    the `/near/` link, open the topic narrow for that stream/topic,
>    and scroll to the message*.
> 3. If the message was *at some point* in the stream and topic
>    encoded in the `/near/` link, according to its `edit_history`,
>    open the topic narrow for the stream/topic that the message is
>    currently in, and scroll to the message*.
> 4. Otherwise (i.e., the message was never in the stream/topic
>    encoded in the `/near/` link), open the topic narrow for the
>    stream/topic encoded in the link, and scroll to the message
>    "nearest" to the message ID*.
>
> *Actually opening a narrow scrolled to a specific message ID is
> blocked on zulip#3604.

It actually turned out quite easy to do the special handling for
part 4, so that's included here. (The bit about blocking on zulip#3604 is
still true.)

Fixes: zulip#5306
dootMaster pushed a commit to dootMaster/zulip-mobile that referenced this issue Sep 28, 2022
@chrisbobbe
api: Add getSingleMessage binding for GET messages/{message_id}
5cc72d4 
We'll use this for zulip#5306; see the plan in discussion:
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M5306.20Follow.20.2Fnear.2F.20links.20through.20topic.20moves.2Frenames/near/1407930

In particular, we want the stream and topic for a stream message
that might not be in our data structures. We'll use this endpoint to
fetch that information.

topic edit modal [nfc]: Add TopicModalProvider context component.

Contains visibility context and handler callback context.
Sets up context for modal handler to be called inside topic action sheets.

topic edit modal [nfc]: Provide topic modal Context hook to children.

The useTopicModalHandler is called normally in TopicItem and TitleStream.
In order to deliver the callbacks to the action sheets in MessageList, the context hook
is called in ChatScreen and a bit of prop-drilling is performed.

topic edit modal: Add translation for action sheet button.

topic edit modal: Add modal and functionality to perform topic name updates.

Fixes zulip#5365

topid edit modal [nfc]: Revise Flow types for relevant components.

topic edit modal: Modify webview unit tests to accommodate feature update.
@gnprice gnprice closed this as completed in 6a6409d Oct 8, 2022
@gnprice gnprice mentioned this issue May 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@chrisbobbe chrisbobbe

Labels
P1 high-priority server release goal Things we should try to coordinate with a major Zulip Server release. webapp parity Features that exist in the webapp that we need to port to mobile. We aren't aiming for full parity. @zulip/shared Good spots to share code with the webapp
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

messagesActions: Follow tapped /near/ links through message moves chrisbobbe/zulip-mobile
2 participants
@gnprice @chrisbobbe